Closed Bug 1133760 Opened 8 years ago Closed 8 years ago

Remove unforgeable holders from DOM proxies

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 5 obsolete files)

Right now, we have a setup for DOM proxies where unforgeable props are stored on a special "unforgeable holder" object that hangs off the proto.

I'm going to remove this and put the props on the object itself (so on the expando object).  This should be fine, because the only proxy we have with unforgeables is HTMLDocument, and it's not like we have tons of those around.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attached patch Diff of HTMLDocumentBinding.cpp (obsolete) — Splinter Review
Apart from the removal of sharedRoot, the only changes to generated code are in HTMLDocumentBinding.cpp and ImageDocumentBinding.cpp.  This is a diff of the former; the latter is much the same.
Attachment #8565491 - Attachment is obsolete: true
Attachment #8565491 - Flags: review?(peterv)
Attached patch Updated HTMLDocument diff (obsolete) — Splinter Review
This has those ReleaseWrapper fixes we discussed on IRC.
Attachment #8565492 - Attachment is obsolete: true
Attachment #8565559 - Attachment is obsolete: true
Attachment #8565559 - Flags: review?(peterv)
This fixes the problem caught by the second part of the test: named getters can interfere with defineProperty on the proxy, so we really do want to define unforgeables directly on the expando object in the proxy case.
Attachment #8565560 - Attachment is obsolete: true
OK.  There is in fact still an issue here, not even counting the bits in bug 928336 that might mean we want to hold on to the holder.

In particular, for the non-proxy case we really do want to define unforgeable stuff before we SetWrapper.  Otherwise the unforgeable bits trigger wrapper preservation...
Oh, and for proxies, simply creating the expando object triggers wrapper preservation.  And there's no other obvious place to put it where it won't be slow.

Maybe we just need to have different ordering for the SetWrapper() calls in the proxy and non-proxy cases. I think that should be ok.
Blocks: 928336
Attachment #8566660 - Attachment is obsolete: true
Attachment #8566660 - Flags: review?(peterv)
Attachment #8567927 - Flags: review?(peterv)
Attachment #8565490 - Flags: review?(peterv) → review+
Attachment #8567927 - Flags: review?(peterv) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.