Last Comment Bug 417617 - DOMParser.parseFromString in Greasemonkey script causes "ASSERTION: Should have inner window here!"
: DOMParser.parseFromString in Greasemonkey script causes "ASSERTION: Should ha...
Status: VERIFIED FIXED
: assertion, regression, testcase, verified1.8.1.13
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9beta4
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on: 430276
Blocks: 403168
  Show dependency treegraph
 
Reported: 2008-02-14 15:30 PST by Jesse Ruderman
Modified: 2008-05-08 12:23 PDT (History)
15 users (show)
dsicore: blocking1.9+
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple greasemonkey script that triggers this bug (244 bytes, text/javascript)
2008-02-14 15:31 PST, Jesse Ruderman
no flags Details
stack trace (11.32 KB, text/plain)
2008-02-14 15:31 PST, Jesse Ruderman
no flags Details
testcase 2 (295 bytes, text/html)
2008-02-15 00:17 PST, moz_bug_r_a4
no flags Details
testcase 3 - XMLHttpRequest (317 bytes, text/html)
2008-02-15 00:54 PST, moz_bug_r_a4
no flags Details
possible patch (1.92 KB, patch)
2008-02-15 06:09 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jst: review+
jst: superreview+
Details | Diff | Review
security checks (13.18 KB, patch)
2008-02-26 13:46 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jst: review+
jst: superreview+
Details | Diff | Review
for 1.8 (10.86 KB, patch)
2008-02-29 07:52 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jst: review+
jst: superreview+
dveditz: approval1.8.1.13+
asac: approval1.8.0.next?
Details | Diff | Review
for 1.8.0 (10.32 KB, patch)
2008-03-25 10:31 PDT, Alexander Sack
no flags Details | Diff | Review

Description Jesse Ruderman 2008-02-14 15:30:18 PST
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 2008-02-14 15:31:10 PST
Created attachment 303391 [details]
simple greasemonkey script that triggers this bug
Comment 2 Jesse Ruderman 2008-02-14 15:31:27 PST
Created attachment 303392 [details]
stack trace
Comment 3 moz_bug_r_a4 2008-02-15 00:16:20 PST
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 2008-02-15 00:17:36 PST
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]"
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2008-02-15 00:35:59 PST
Blake, Johnny, how bad is this?
Comment 6 moz_bug_r_a4 2008-02-15 00:54:51 PST
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]"
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-15 02:08:54 PST
So in this case we're calling nsWindowSH::GlobalResolve on outer window.
Do we really want to do that?
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-15 02:51:39 PST
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] (high review load, please consider other reviewers) 2008-02-15 06:09:09 PST
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?
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-21 05:12:56 PST
Blake, Jst, any comments?
Comment 11 Damon Sicore (:damons) 2008-02-21 14:15:41 PST
+'ing with P2.  JST, Blake, Jonas, please re-prioritize as necessary. 
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2008-02-21 17:43:33 PST
Yeah, we need to fix this, having an outer window at that point in the document is bad news.
Comment 13 Daniel Veditz [:dveditz] 2008-02-22 12:16:17 PST
We'll consider this for the branch once the trunk patch is in.
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-25 12:06:26 PST
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 Johnny Stenback (:jst, jst@mozilla.com) 2008-02-25 23:27:08 PST
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] (high review load, please consider other reviewers) 2008-02-25 23:38:48 PST
That is exactly what is happening with the testcase, DOMConstructor get called
with outer window, apparently because of XPCNativeWrapper.
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-25 23:42:20 PST
Hmm, actually in testcase  the inner window should be the right one...
Ok, I'll add some check there.
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-26 13:46:07 PST
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.
Comment 19 Reed Loden [:reed] (use needinfo?) 2008-02-26 18:03:49 PST
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] (high review load, please consider other reviewers) 2008-02-28 14:53:12 PST
Since this is a regression from a patch which is in .13, taking the fix there would be good.
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-02-29 07:52:25 PST
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.
Comment 22 Daniel Veditz [:dveditz] 2008-02-29 16:12:03 PST
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 [:mwargers] (not working for Mozilla) 2008-03-12 02:28:01 PDT
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 2008-03-12 19:29:12 PDT
(In reply to comment #23)
> That is what is wanted, right? 
> 

Yes.
Comment 25 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-03-13 04:35:03 PDT
Ok, thanks for the info, marking verified1.8.1.13 then.
Comment 26 Alexander Sack 2008-03-25 10:31:44 PDT
Created attachment 311614 [details] [diff] [review]
for 1.8.0

just some minor context adjustments.
Comment 27 Alexander Sack 2008-03-25 11:59:28 PDT
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] 2008-05-08 12:23:54 PDT
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

Note You need to log in before you can comment on or make changes to this bug.