Check for sanity in DOM slot handling got removed

RESOLVED FIXED in Firefox 55



JavaScript Engine
8 months ago
8 months ago


(Reporter: bz, Assigned: bz)


Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)



(1 attachment) 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
Created attachment 8863420 [details] [diff] [review]
Restore check for sanity of slot indices on DOM objects that got lost
Attachment #8863420 - Flags: review?(kyle)
Attachment #8863420 - Flags: review?(kyle) → review+

Comment 5

8 months ago
Pushed by
Restore check for sanity of slot indices on DOM objects that got lost.  r=qdot

Comment 6

8 months ago
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.