Page MenuHomeWildfire Games

Update ObjectToIDMap before upgrading SpiderMonkey to 52
Changes PlannedPublic

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

Details

Reviewers
None
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.Thu, Jul 16, 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.EditedFri, Jul 17, 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.Fri, Jul 17, 9:09 AM
asterix updated the Trac tickets for this revision.Fri, Jul 17, 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.