Page MenuHomeWildfire Games

Update ObjectToIDMap before upgrading SpiderMonkey to 52
Changes PlannedPublic

Authored by Itms on Jul 16 2020, 9:51 PM.

Details

Reviewers
Vulcan
Trac Tickets
#4893
Summary

When upgrading to SpiderMonkey 45, I forgot to update ObjectToIDMap, which is based on upstream's js/ipc/JavaScriptShared files.

This change is the equivalent of upstream's commit: https://hg.mozilla.org/mozilla-central/rev/fa3bffbe3ec8

As of now, I kept the patch minimal and I preserved the call to JS_AddExtraGCRootsTracer. I believe it would be equivalent to keep the Table inside a JS::PermanentRooted. We could even get rid of this whole class and just make a typedef.


However, the current patch doesn't work. This does not build on MSVC and it segfaults on GCC.

As far as I understand, this is because the implementation of MovableCellHasher<JS::Heap<T>> is broken. In js/public/RootingAPI.h, we can read

template <typename T>
struct JS_PUBLIC_API(MovableCellHasher<JS::Heap<T>>)
{
    using Key = JS::Heap<T>;
    using Lookup = T;
[...]
    static void rekey(Key& k, const Key& newKey) { k.unsafeSet(newKey); }
};

However, JS::Heap<T> does not have a unsafeSet method.


Right now, this bug prevents me from testing SM52. I would prefer to figure it out under SM45 in order to make the upgrade less painful.

Test Plan

Right now I'm just uploading this in order to be able to chat with SpiderMonkey developers.

Event Timeline

Itms planned changes to this revision.Jul 16 2020, 9:51 PM
Itms created this revision.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2672/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1040/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2139/display/redirect

asterix added subscribers: Bellaz89, asterix.

Does @Bellaz89 run into similar issue? I believe sometime back something was discussed in irc.

Stan removed a reviewer: Stan.EditedJul 17 2020, 9:09 AM

Right now I'm just uploading this in order to be able to chat with SpiderMonkey developers.

I do not believe I can do anything to help so I am removing myself.

Stan added a subscriber: Stan.Jul 17 2020, 9:09 AM
asterix updated the Trac tickets for this revision.Jul 17 2020, 9:13 AM

Thanks for the help asterix but this is not reviewable, I just uploaded it to be able to show it to other devs. In his branch, Bellaz changed the key type slightly, which could be a possible fix but does not match upstream. I am going to try a different way based on my discussion with SM devs.

I just want to point out that this class is mostly used by the code changed in D2746 (which still uses it).

@asterix @Itms

This ObjectToID map was one of the most annoying things to change. The changes done on my branch worked but I tested them only on Linux.
Since there is no real documentation on the interface I just did attempts until it worked. So I am not sure if it was the correct way to do it.

I also modified test_ObjectToIDMap.h removing the tests on naked SM object pointers.
I did so because, in SM doc, it is explicitly said that a naked pointer should not be used after code that might trigger a GC collection.

Itms added a comment.Aug 7 2020, 11:05 AM

I am testing a few Jenkins improvements on this diff, sorry for the noise!

Vulcan requested changes to this revision.Aug 7 2020, 11:07 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/tests/23/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/tests/32/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/tests/33/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

builderr-debug-gcc6.txt
In file included from ../../../source/simulation2/serialization/BinarySerializer.h:23:0,
                 from ../../../source/network/NetMessageSim.cpp:24:
../../../source/scriptinterface/third_party/ObjectToIDMap.h:30:12: error: specialization of 'template<class> struct js::DefaultGCPolicy' in different namespace [-fpermissive]
 struct js::DefaultGCPolicy<std::wstring> : public js::IgnoreGCPolicy<std::wstring> {};
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../../libraries/source/spidermonkey/include-unix-debug/js/TraceableVector.h:13:0,
                 from ../../../libraries/source/spidermonkey/include-unix-debug/jsapi.h:33,
                 from ../../../source/scriptinterface/ScriptTypes.h:61,
                 from ../../../source/scriptinterface/ScriptVal.h:21,
                 from ../../../source/network/NetMessages.h:27,
                 from ../../../source/network/NetMessage.h:26,
                 from ../../../source/network/NetMessageSim.cpp:20:
../../../libraries/source/spidermonkey/include-unix-debug/js/TracingAPI.h:375:8: error:   from definition of 'template<class> struct js::DefaultGCPolicy' [-fpermissive]
 struct DefaultGCPolicy;
        ^~~~~~~~~~~~~~~
In file included from ../../../source/simulation2/serialization/BinarySerializer.h:23:0,
                 from ../../../source/network/NetMessageSim.cpp:24:
../../../source/scriptinterface/third_party/ObjectToIDMap.h: In member function 'bool ObjectIdCache<T>::find(JSObject*, T&)':
../../../source/scriptinterface/third_party/ObjectToIDMap.h:75:3: error: need 'typename' before 'ObjectIdCache<T>::Table:: Ptr' because 'ObjectIdCache<T>::Table' is a dependent scope
   Table::Ptr p = table_.lookup(obj);
   ^~~~~
../../../source/scriptinterface/third_party/ObjectToIDMap.h:75:14: error: expected ';' before 'p'
   Table::Ptr p = table_.lookup(obj);
              ^
../../../source/scriptinterface/third_party/ObjectToIDMap.h:76:8: error: 'p' was not declared in this scope
   if (!p)
        ^
../../../source/scriptinterface/third_party/ObjectToIDMap.h:78:9: error: 'p' was not declared in this scope
   ret = p->value();
         ^
make[1]: *** [obj/network_Debug/NetMessageSim.o] Error 1
make: *** [network] Error 2

Link to build: https://jenkins.wildfiregames.com/job/tests/34/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/tests/37/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

builderr-debug-gcc6.txt
In file included from ../../../source/simulation2/serialization/BinarySerializer.h:23:0,
                 from ../../../source/network/NetMessageSim.cpp:24:
../../../source/scriptinterface/third_party/ObjectToIDMap.h:30:12: error: specialization of 'template<class> struct js::DefaultGCPolicy' in different namespace [-fpermissive]
 struct js::DefaultGCPolicy<std::wstring> : public js::IgnoreGCPolicy<std::wstring> {};
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../../libraries/source/spidermonkey/include-unix-debug/js/TraceableVector.h:13:0,
                 from ../../../libraries/source/spidermonkey/include-unix-debug/jsapi.h:33,
                 from ../../../source/scriptinterface/ScriptTypes.h:61,
                 from ../../../source/scriptinterface/ScriptVal.h:21,
                 from ../../../source/network/NetMessages.h:27,
                 from ../../../source/network/NetMessage.h:26,
                 from ../../../source/network/NetMessageSim.cpp:20:
../../../libraries/source/spidermonkey/include-unix-debug/js/TracingAPI.h:375:8: error:   from definition of 'template<class> struct js::DefaultGCPolicy' [-fpermissive]
 struct DefaultGCPolicy;
        ^~~~~~~~~~~~~~~
In file included from ../../../source/simulation2/serialization/BinarySerializer.h:23:0,
                 from ../../../source/network/NetMessageSim.cpp:24:
../../../source/scriptinterface/third_party/ObjectToIDMap.h: In member function 'bool ObjectIdCache<T>::find(JSObject*, T&)':
../../../source/scriptinterface/third_party/ObjectToIDMap.h:75:3: error: need 'typename' before 'ObjectIdCache<T>::Table:: Ptr' because 'ObjectIdCache<T>::Table' is a dependent scope
   Table::Ptr p = table_.lookup(obj);
   ^~~~~
../../../source/scriptinterface/third_party/ObjectToIDMap.h:75:14: error: expected ';' before 'p'
   Table::Ptr p = table_.lookup(obj);
              ^
../../../source/scriptinterface/third_party/ObjectToIDMap.h:76:8: error: 'p' was not declared in this scope
   if (!p)
        ^
../../../source/scriptinterface/third_party/ObjectToIDMap.h:78:9: error: 'p' was not declared in this scope
   ret = p->value();
         ^
make[1]: *** [obj/network_Debug/NetMessageSim.o] Error 1
make: *** [network] Error 2

Link to build: https://jenkins.wildfiregames.com/job/tests/39/display/redirect