Closed Bug 420603 Opened 12 years ago Closed 12 years ago

[FIX] Don't run script while setting up proto chain


(Core :: XPConnect, defect, P2)






(Reporter: sicking, Assigned: jst)


(Whiteboard: [sg:critical])


(1 file)

Once bug 401155 has landed the only (known) thing that can cause scripts to execute is while we set up the proto chain for the newly created wrapper.

Currently if the global property HTMLElement has a getter we'll execute that when wrapping any html element.

First off, does it really make sense to replace the prototype objects at all? Adding properties to them does make sense, but it seems like replacing them would just make things fail horribly.

Can we make them read-only properties? That would avoid people accidentally replacing them and making all new wrappers be borked.

At the very least we should make the code that sets up proto chains bail if any of the properties we're getting have getters and skip the rest of the proto chain.

I guess we could do this in a dot-release, but since it could in theory affect content (especially if we do the read-only thing) it would be really good to get into the release.
Flags: blocking1.9+
Whiteboard: [sg:critical]
The first sentence should say:

that can cause scripts to execute *while wrapping* is while we set up the proto
Attached patch Fix.Splinter Review
This fixes the cases I've been able to find that might run scripts while we're wrapping a node. If others know of other places or know how to test for this I'd love to hear about it.
Attachment #309524 - Flags: superreview?(peterv)
Attachment #309524 - Flags: review?(mrbkap)
Summary: Don't run script while setting up proto chain → [FIX] Don't run script while setting up proto chain
Not going to block on this one, but I'd still appreciate reviews...
Flags: blocking1.9+ → blocking1.9-
Attachment #309524 - Flags: review?(mrbkap) → review+
Attachment #309524 - Flags: superreview?(peterv) → superreview+
Attachment #309524 - Flags: approval1.9+
Fixed. Note that the last hunk of that diff (formatting change only) doesn't apply against the trunk and should be ignored, it only applies against a local change here.
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.