Closed Bug 295122 Opened 19 years ago Closed 19 years ago

contentWindow.location (and href) throws DOM security error

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: bugzilla-mozilla-20000923, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

Since the latest change in bug 281988, ChatZilla's been having a little problem
with getting the location of its content windows.

To get the error, in any very recent Firefox build, open ChatZilla and do this:
  /eval this.frame.contentWindow.location.href

It seems  /eval this.frame.contentWindow shows that the Window object is being
wrapped by XPCNativeWrapper (which I'm told is perfectly correct), however the
exception is not.
The problem here is that we end up in XPC_NW_NewResolve, decide we need to
delegate to the unwrapped object, go to do the OBJ_DEFINE_PROPERTY() property
thing.  This calls into nsWindowSH::AddProperty, which throws for the "location"
property.  So just getting window.location fails.

So the problem, it seems, is that the 

  // All we need to do is define the property in obj if it exists in
  // the wrapped native's object.

comment isn't what we're doing.  We're trying to define the property in the
wrapped native (because our AddProperty hook just passes things along here).

I tried just skipping the OBJ_DEFINE_PROPERTY call, but that breaks other things...

Perhaps we should consider forwarding to the other class hook first, and if that
doesn't resolve anything doing what we do now?  Or would that not work?
Flags: blocking1.8b2?
OS: Windows 2000 → All
Hardware: PC → All
Blocks: 295040
Attached patch fix? (obsolete) — Splinter Review
Not enough... document.location has the same issue.  And the AddProperty hook on
nsNodeSH does weird wrapper-preserve stuff, like I said on IRC.

It really feels like we don't want to be calling AddProperty here when the
property is "already there".  The question is whether we can detect this last case.

I guess we can do this for now if we have no better ideas...
brendan says r+a=him
Attachment #184257 - Attachment is obsolete: true
Attachment #184262 - Flags: review+
Assignee: general → bzbarsky
Target Milestone: --- → mozilla1.8beta2
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified in trunk Firefox, dated 2005-05-22 23:30:05.76 BST.
Status: RESOLVED → VERIFIED
I'd like to get this in now, since we know the JSClass.delProperty forwarding
is not useful (delProperty, like addProperty, is a notification callback that
does not actually remove the id'd property).

/be
Attachment #184290 - Flags: review?(bzbarsky)
Attachment #184290 - Flags: approval1.8b2+
Comment on attachment 184290 [details] [diff] [review]
followup fix for correct delete property bypass

r+sr=jst
Attachment #184290 - Flags: superreview+
Attachment #184290 - Flags: review?(bzbarsky)
Attachment #184290 - Flags: review+
Comment on attachment 184290 [details] [diff] [review]
followup fix for correct delete property bypass

Checked in, thanks.

/be
Flags: blocking1.8b2?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: