Remove location property protection (sDoSecurityCheckInAddProperty).

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
Created attachment 377106 [details] [diff] [review]
Nuke it.

With all the wrapper protection we have now there's no real need for sDoSecurityCheckInAddProperty any longer. It used to be important to prevent pages from setting a getter or a setter on window.location, but that no longer matters as all they could do is to fool other pages in the same origin, or an extension that does window.wrappedJSObject.location... We'd save some on complexity and bug prone code if we'd just remove the protection.
Attachment #377106 - Flags: superreview?(mrbkap)
Attachment #377106 - Flags: review?(mrbkap)
(Assignee)

Comment 1

8 years ago
Oh, and that patch also fixes a bug in nsDocumentSH::AddProperty() where it wasn't forwarding the call to nsEventReceiverSH::AddProperty(), like nsNodeSH::AddProperty() does (but note that we shouldn't forward to nsNodeSH, as its AddProperty() hook also calls PreserveNodeWrapper() which we don't want called for document objects).
> but note that we shouldn't forward to nsNodeSH,
> as its AddProperty() hook also calls PreserveNodeWrapper() which we don't want
> called for document objects

That should be a comment in the code, imo.
Comment on attachment 377106 [details] [diff] [review]
Nuke it.

r+sr=mrbkap with that comment.
Attachment #377106 - Flags: superreview?(mrbkap)
Attachment #377106 - Flags: superreview+
Attachment #377106 - Flags: review?(mrbkap)
Attachment #377106 - Flags: review+
(Assignee)

Comment 4

8 years ago
(In reply to comment #2)
> > but note that we shouldn't forward to nsNodeSH,
> > as its AddProperty() hook also calls PreserveNodeWrapper() which we don't want
> > called for document objects
> 
> That should be a comment in the code, imo.

Indeed it should.
(Assignee)

Comment 5

8 years ago
Fixed.

http://hg.mozilla.org/mozilla-central/rev/65e30d612773
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 541530
You need to log in before you can comment on or make changes to this bug.