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

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: simple greasemonkey script that triggers this bug

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[moz_bug_r_a4]

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

[moz_bug_r_a4]

Attachment: 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]"

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Blake, Johnny, how bad is this?

Comment 6

[moz_bug_r_a4]

Attachment: testcase 3 - XMLHttpRequest

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]"

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

So in this case we're calling nsWindowSH::GlobalResolve on outer window.
Do we really want to do that?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Blake, Jst, any comments?

Comment 11

[Damon Sicore (:damons)]

+'ing with P2.  JST, Blake, Jonas, please re-prioritize as necessary. 

Comment 12

[Johnny Stenback (:jst)]

Yeah, we need to fix this, having an outer window at that point in the document is bad news.

Comment 13

[Daniel Veditz [:dveditz]]

We'll consider this for the branch once the trunk patch is in.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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.

[e-mail], can you find any problems with the patch?

Comment 15

[Johnny Stenback (:jst)]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

That is exactly what is happening with the testcase, DOMConstructor get called
with outer window, apparently because of XPCNativeWrapper.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Hmm, actually in testcase  the inner window should be the right one...
Ok, I'll add some check there.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: security checks

This adds security checks if window is outer window.
Caller must have right principal or right capability.

Comment 19

[Reed Loden [:reed]]

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

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Since this is a regression from a patch which is in .13, taking the fix there would be good.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 306518 [details] [diff] [review]
for 1.8

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 23

[Martijn Wargers (dead)]

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

[moz_bug_r_a4]

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

Yes.

Comment 25

[Martijn Wargers (dead)]

Ok, thanks for the info, marking verified1.8.1.13 then.

Comment 26

[Alexander Sack]

Attachment: for 1.8.0

just some minor context adjustments.

Comment 27

[Alexander Sack]

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.

Comment 28

[Carsten Book [:Tomcat]]

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