Closed Bug 1358596 Opened 7 years ago Closed 7 years ago

Check for sanity in DOM slot handling got removed

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

https://hg.mozilla.org/mozilla-central/rev/35ba8c31d7f3 removes js::shadow::Object::MAX_FIXED_SLOTS and the DOM assert that used it.

But that assert is critical: We have jit code that depends on it.  Specifically, CodeGenerator::visitGetDOMMemberV and CodeGenerator::visitGetDOMMemberT both assume the slot is a fixed slot.  The assert used to assert that the slot index was small enough that this was true.  At the moment nothing prevents the DOM from ending up with a slot index that doesn't fit in fixed slots here...

We can either reinstate some constant in jsfriendapi and reinstate the static_assert, or we can change visitGetDOMMemberT/V to compare the index to NativeObject::MAX_FIXED_SLOTS like CodeGenerator::visitGetDOMProperty does and hence work with dynamic slots too.

Jan, do you have a preference?

Note that this is independent of bug 1237504 except insofar as with that bug fixed MAX_FIXED_SLOTS will make sense for proxies too, not just native objects, and that we may want the jsfriendapi constant at that point anyway.
Flags: needinfo?(jdemooij)
Blocks: 1073842
What if we do both? The assert on the DOM side seems important because we may not have tests hitting the Ion fast path for all DOM properties and the assert in CodeGenerator is nice as that's where we depend on the invariant (it could help other embeddings like Servo for instance).
Flags: needinfo?(jdemooij)
Oh sorry, you suggested fixing visitGetDOMMemberT/V instead of adding an assert there.

Fixing this for visitGetDOMMemberT is not entirely trivial because if the result is a FP register we don't have a scratch register to load the slots pointer into. We could add a temp register to handle that case though.

I don't have a strong preference but I'm happy to do the GetDOMMember* patching if you want.
I'm pretty happy to make this a static assert on the DOM side for the moment.  I just need jsfriendapi exposing the relevant constant.  But I can add add that too, if you're ok with the general idea.
Assignee: nobody → bzbarsky
Depends on: 1360523
Attachment #8863420 - Flags: review?(kyle) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62bbfd73ae6
Restore check for sanity of slot indices on DOM objects that got lost.  r=qdot
https://hg.mozilla.org/mozilla-central/rev/f62bbfd73ae6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: