Page MenuHomeWildfire Games
Differential D2881

Update ObjectToIDMap before upgrading SpiderMonkey to 52
AbandonedPublic
Actions

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.

Diff Detail

Event Timeline

Itms planned changes to this revision.Jul 16 2020, 9:51 PM
Itms created this revision.
Vulcan added a comment.Jul 16 2020, 9:52 PM

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

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

Harbormaster failed remote builds in B12442: Diff 12728!Jul 16 2020, 9:52 PM
Vulcan added a comment.Jul 16 2020, 9:52 PM

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

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

Vulcan added a comment.Jul 16 2020, 9:53 PM

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 reviewers: wraitii, Stan, Bellaz89.Jul 17 2020, 9:06 AM
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
Itms removed reviewers: wraitii, Bellaz89.Jul 17 2020, 9:17 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.

wraitii added a subscriber: wraitii.Jul 17 2020, 9:52 AM

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

Bellaz89 added a comment.Jul 17 2020, 2:26 PM

@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 mentioned this in D2746: Correctly serialize/deserialize user-defined JS objects..Jul 20 2020, 5:03 PM
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

Vulcan added a comment.Aug 7 2020, 12:22 PM

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

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

Vulcan added a comment.Aug 7 2020, 12:27 PM

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

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

Vulcan added a comment.Aug 7 2020, 12:30 PM

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

Vulcan added a comment.Aug 10 2020, 7:43 AM

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

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

Vulcan added a comment.Aug 10 2020, 7:48 AM

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

Itms abandoned this revision.Jan 25 2021, 6:11 PM

Superseded by SM upgrades.

wraitii added a comment.Jan 25 2021, 6:15 PM

(Refs https://code.wildfiregames.com/rP24563)

Revision Contents
Changeset List

PathSize
libraries/
source/
spidermonkey/
37 lines
6 lines
source/
scriptinterface/
16 lines
21 lines
tests/
2 lines
third_party/
76 lines
simulation2/
components/
2 lines
serialization/
2 lines

Diff 12728

View Options

libraries/source/spidermonkey/ExportMovableCellHasher.diff

Loading...
View Options

libraries/source/spidermonkey/patch.sh

Loading...
View Options

source/scriptinterface/ScriptRuntime.h

Loading...
View Options

source/scriptinterface/ScriptRuntime.cpp

Loading...
View Options

source/scriptinterface/tests/test_ObjectToIDMap.h

Loading...
View Options

source/scriptinterface/third_party/ObjectToIDMap.h

Loading...
View Options

source/simulation2/components/CCmpAIManager.cpp

Loading...
View Options

source/simulation2/serialization/BinarySerializer.cpp

Loading...
Wildfire Games · Phabricator