Closed
Bug 417617
Opened 16 years ago
Closed 16 years ago
DOMParser.parseFromString in Greasemonkey script causes "ASSERTION: Should have inner window here!"
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.9beta4
People
(Reporter: jruderman, Assigned: smaug)
References
Details
(4 keywords)
Attachments
(8 files)
244 bytes,
text/javascript
|
Details | |
11.32 KB,
text/plain
|
Details | |
295 bytes,
text/html
|
Details | |
317 bytes,
text/html
|
Details | |
1.92 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
13.18 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
10.86 KB,
patch
|
jst
:
review+
jst
:
superreview+
dveditz
:
approval1.8.1.13+
asac
:
approval1.8.0.next?
|
Details | Diff | Splinter Review |
10.32 KB,
patch
|
Details | Diff | Splinter Review |
Doing this from a Greasemonkey script: (new DOMParser()).parseFromString('<root/>', 'text/xml'); makes Firefox assert in nsDocument::SetScriptHandlingObject: ###!!! ASSERTION: Should have inner window here!: '!win || win->IsInnerWindow()', file /Users/jruderman/trunk/mozilla/content/base/src/nsDocument.cpp, line 2583 This assertion was added in bug 403168. I noticed this bug because my "Valid XHTML" user script triggers it. I'm using Firefox trunk on Mac with Greasemonkey 0.7.20080121.0.
Reporter | ||
Comment 1•16 years ago
|
||
Reporter | ||
Comment 2•16 years ago
|
||
Comment 3•16 years ago
|
||
XPCNativeWrapper seems to be related. On fx-2.0.0.13pre-2008-02-13-04, the following code fails (w/o Greasemonkey). var w = new XPCNativeWrapper(window); (new w.DOMParser()).parseFromString('<root/>', 'text/xml');
Comment 4•16 years ago
|
||
fx-2.0.0.13pre-2008-02-12-03: [object XPCNativeWrapper [object XMLDocument]] fx-2.0.0.13pre-2008-02-13-04: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMParser.parseFromString]"
Blake, Johnny, how bad is this?
Flags: blocking1.9?
Comment 6•16 years ago
|
||
MLHttpRequest is also affected. var w = new XPCNativeWrapper(window); var x = (new w.XMLHttpRequest()); x.open("GET", location.href, true); // fails both on trunk and fx2 fx-3.0b4pre-2008-02-13-04: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.open]" fx-2.0.0.13pre-2008-02-13-04: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIXMLHttpRequest.open]"
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 7•16 years ago
|
||
So in this case we're calling nsWindowSH::GlobalResolve on outer window. Do we really want to do that?
Assignee | ||
Comment 8•16 years ago
|
||
XHR and DOMParser should be bound to inner window. If someone has access to outer window only, would it be ok to call nsDOMConstructor using inner window as a parameter? Or would that lead to some security problems?
Flags: blocking1.8.1.13?
Assignee | ||
Updated•16 years ago
|
Priority: -- → P1
Assignee | ||
Updated•16 years ago
|
Group: security
Assignee | ||
Comment 9•16 years ago
|
||
This is the possible patch, but have to ensure that if someone is using XPCNativeWrapper (or otherwise getting outer window), this doesn't open new security holes. So far I haven't had found any problems. The BaseStubConstructor part may not be absolutely necessary, but if XPCNativeWrapper is used to create XHR, and the page for which XPCNativeWrapper went away, that NS_ENSURE_STATE is false. However, even without that the created XHR is non-functional. But perhaps better to throw the error as soon as possible. Blake, Jst, is this reasonable? How are XPCNativeWrappers handled if inner window goes away?
Assignee | ||
Comment 10•16 years ago
|
||
Blake, Jst, any comments?
Comment 11•16 years ago
|
||
+'ing with P2. JST, Blake, Jonas, please re-prioritize as necessary.
Flags: blocking1.9? → blocking1.9+
Priority: P1 → P2
Comment 12•16 years ago
|
||
Yeah, we need to fix this, having an outer window at that point in the document is bad news.
Comment 13•16 years ago
|
||
We'll consider this for the branch once the trunk patch is in.
Flags: blocking1.8.1.13? → wanted1.8.1.x+
Assignee | ||
Comment 14•16 years ago
|
||
So jst, what do you think about the patch? I tried to write some testcase which could break it, but so far didn't find any problems. Would be great to have this fixed asap, so that it could be get into .13 too. moz_bug_r_a4@yahoo.com, can you find any problems with the patch?
Comment 15•16 years ago
|
||
Comment on attachment 303525 [details] [diff] [review] possible patch nsDOMConstructor(const PRUnichar *aName, const nsGlobalNameStruct *aNameStruct, - nsISupports* aOwner) + nsPIDOMWindow* aOwner) : mClassName(aName), mConstructable(IsConstructable(aNameStruct)), - mWeakOwner(do_GetWeakReference(aOwner)) + mWeakOwner(do_GetWeakReference(aOwner->IsOuterWindow() ? + aOwner->GetCurrentInnerWindow() : aOwner)) { } I'm curious to know if we ever get in here with aOwner being an outer? If so, we should make sure that can only happen in cases where we know it's ok, having aOwner be an outer and this call being initiated from an other other than the current inner could be bad. Maybe we could even throw here if aOwner is an outer window? r+sr=jst with that figured out.
Attachment #303525 -
Flags: superreview+
Attachment #303525 -
Flags: review+
Assignee | ||
Comment 16•16 years ago
|
||
That is exactly what is happening with the testcase, DOMConstructor get called with outer window, apparently because of XPCNativeWrapper.
Assignee | ||
Comment 17•16 years ago
|
||
Hmm, actually in testcase the inner window should be the right one... Ok, I'll add some check there.
Assignee | ||
Comment 18•16 years ago
|
||
This adds security checks if window is outer window. Caller must have right principal or right capability.
Attachment #305837 -
Flags: superreview?(jst)
Attachment #305837 -
Flags: review?(jst)
Updated•16 years ago
|
Attachment #305837 -
Flags: superreview?(jst)
Attachment #305837 -
Flags: superreview+
Attachment #305837 -
Flags: review?(jst)
Attachment #305837 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 19•16 years ago
|
||
Checking in content/base/public/nsContentUtils.h; /cvsroot/mozilla/content/base/public/nsContentUtils.h,v <-- nsContentUtils.h new revision: 1.166; previous revision: 1.165 done Checking in content/base/src/nsContentUtils.cpp; /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v <-- nsContentUtils.cpp new revision: 1.284; previous revision: 1.283 done Checking in dom/src/base/nsDOMClassInfo.cpp; /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v <-- nsDOMClassInfo.cpp new revision: 1.525; previous revision: 1.524 done
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Assignee | ||
Comment 20•16 years ago
|
||
Since this is a regression from a patch which is in .13, taking the fix there would be good.
Flags: blocking1.8.1.13?
Assignee | ||
Comment 21•16 years ago
|
||
Since nsContentUtils::CanCallerAccess(nsIDOMNode*) isn't the same on branch as it is on trunk, the changes to nsContentUtils aren't exactly the same in this patch as they were in the patch for trunk. nsContentUtils::CanCallerAccess(nsPIDOMWindow*) is almost copy-paste of nsContentUtils::CanCallerAccess(nsIDOMNode*), had to only change where to get the principal.
Attachment #306518 -
Flags: superreview?(jst)
Attachment #306518 -
Flags: review?(jst)
Attachment #306518 -
Flags: approval1.8.1.13?
Updated•16 years ago
|
Attachment #306518 -
Flags: superreview?(jst)
Attachment #306518 -
Flags: superreview+
Attachment #306518 -
Flags: review?(jst)
Attachment #306518 -
Flags: review+
Comment 22•16 years ago
|
||
Comment on attachment 306518 [details] [diff] [review] for 1.8 approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #306518 -
Flags: approval1.8.1.13? → approval1.8.1.13+
Updated•16 years ago
|
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.8.1.13
Comment 23•16 years ago
|
||
I'm getting "[object XPCNativeWrapper [object XMLDocument @ 0x120dc358 (native @ 0x12e11fe0)]]" for the 2nd testcase, and "ok" for the 3rd testcase on branch. That is what is wanted, right?
Comment 24•16 years ago
|
||
(In reply to comment #23) > That is what is wanted, right? > Yes.
Comment 25•16 years ago
|
||
Ok, thanks for the info, marking verified1.8.1.13 then.
Keywords: fixed1.8.1.13 → verified1.8.1.13
Updated•16 years ago
|
Flags: blocking1.8.0.15+
Comment 26•16 years ago
|
||
just some minor context adjustments.
Comment 27•16 years ago
|
||
Comment on attachment 306518 [details] [diff] [review] for 1.8 appears to fix the greasemonkey regression for us. use the 1.8.0 patch attached for a clean commit.
Attachment #306518 -
Flags: approval1.8.0.15?
Updated•16 years ago
|
Group: security
Updated•16 years ago
|
Flags: in-testsuite?
Depends on: 430276
Comment 28•16 years ago
|
||
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre and the testcases from this bug
Status: RESOLVED → VERIFIED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•