DOMParser.parseFromString in Greasemonkey script causes "ASSERTION: Should have inner window here!"

VERIFIED FIXED in mozilla1.9beta4

Status

()

Core
DOM
P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(4 keywords)

Trunk
mozilla1.9beta4
assertion, regression, testcase, verified1.8.1.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 303391 [details]
simple greasemonkey script that triggers this bug
(Reporter)

Comment 2

10 years ago
Created attachment 303392 [details]
stack trace

Comment 3

10 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

10 years ago
Created attachment 303478 [details]
testcase 2

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

10 years ago
Created attachment 303487 [details]
testcase 3 - XMLHttpRequest

XMLHttpRequest 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

10 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 7

10 years ago
So in this case we're calling nsWindowSH::GlobalResolve on outer window.
Do we really want to do that?
(Assignee)

Comment 8

10 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

10 years ago
Priority: -- → P1
(Assignee)

Updated

10 years ago
Group: security
(Assignee)

Comment 9

10 years ago
Created attachment 303525 [details] [diff] [review]
possible patch

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

10 years ago
Blake, Jst, any comments?
+'ing with P2.  JST, Blake, Jonas, please re-prioritize as necessary. 
Flags: blocking1.9? → blocking1.9+
Priority: P1 → P2
Yeah, we need to fix this, having an outer window at that point in the document is bad news.
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

10 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 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

10 years ago
That is exactly what is happening with the testcase, DOMConstructor get called
with outer window, apparently because of XPCNativeWrapper.
(Assignee)

Comment 17

10 years ago
Hmm, actually in testcase  the inner window should be the right one...
Ok, I'll add some check there.
(Assignee)

Comment 18

10 years ago
Created attachment 305837 [details] [diff] [review]
security checks

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

10 years ago
Attachment #305837 - Flags: superreview?(jst)
Attachment #305837 - Flags: superreview+
Attachment #305837 - Flags: review?(jst)
Attachment #305837 - Flags: review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Last Resolved: 10 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Comment 20

9 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

9 years ago
Created attachment 306518 [details] [diff] [review]
for 1.8

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

9 years ago
Attachment #306518 - Flags: superreview?(jst)
Attachment #306518 - Flags: superreview+
Attachment #306518 - Flags: review?(jst)
Attachment #306518 - Flags: review+
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+
Flags: blocking1.8.1.13? → blocking1.8.1.13+
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.13
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

9 years ago
(In reply to comment #23)
> That is what is wanted, right? 
> 

Yes.
Ok, thanks for the info, marking verified1.8.1.13 then.
Keywords: fixed1.8.1.13 → verified1.8.1.13

Updated

9 years ago
Flags: blocking1.8.0.15+

Comment 26

9 years ago
Created attachment 311614 [details] [diff] [review]
for 1.8.0

just some minor context adjustments.

Comment 27

9 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?
Group: security
Flags: in-testsuite?
Depends on: 430276
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
You need to log in before you can comment on or make changes to this bug.