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...
: 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]
: Andrew Overholt [:overholt]
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+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
security checks (13.18 KB, patch)
2008-02-26 13:46 PST, Olli Pettay [:smaug]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
for 1.8 (10.86 KB, patch)
2008-02-29 07:52 PST, Olli Pettay [:smaug]
jst: review+
jst: superreview+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review
for 1.8.0 (10.32 KB, patch)
2008-03-25 10:31 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description User image 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 User image Jesse Ruderman 2008-02-14 15:31:10 PST
Created attachment 303391 [details]
simple greasemonkey script that triggers this bug
Comment 2 User image Jesse Ruderman 2008-02-14 15:31:27 PST
Created attachment 303392 [details]
stack trace
Comment 3 User image moz_bug_r_a4 2008-02-15 00:16:20 PST
XPCNativeWrapper seems to be related.

On fx-, the following code fails (w/o Greasemonkey).

  var w = new XPCNativeWrapper(window);
  (new w.DOMParser()).parseFromString('<root/>', 'text/xml');
Comment 4 User image moz_bug_r_a4 2008-02-15 00:17:36 PST
Created attachment 303478 [details]
testcase 2

[object XPCNativeWrapper [object XMLDocument]]

[Exception... "Component returned failure code: 0x8000ffff 
(NS_ERROR_UNEXPECTED) [nsIDOMParser.parseFromString]"
Comment 5 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-15 00:35:59 PST
Blake, Johnny, how bad is this?
Comment 6 User image 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());"GET", location.href, true); // fails both on trunk and fx2

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 
Comment 7 User image Olli Pettay [:smaug] 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 User image Olli Pettay [:smaug] 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 User image Olli Pettay [:smaug] 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 User image Olli Pettay [:smaug] 2008-02-21 05:12:56 PST
Blake, Jst, any comments?
Comment 11 User image Damon Sicore (:damons) 2008-02-21 14:15:41 PST
+'ing with P2.  JST, Blake, Jonas, please re-prioritize as necessary. 
Comment 12 User image Johnny Stenback (:jst, 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 User image 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 User image Olli Pettay [:smaug] 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., can you find any problems with the patch?
Comment 15 User image Johnny Stenback (:jst, 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),
-      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 User image Olli Pettay [:smaug] 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 User image Olli Pettay [:smaug] 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 User image Olli Pettay [:smaug] 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 User image 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
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
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
Comment 20 User image Olli Pettay [:smaug] 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 User image Olli Pettay [:smaug] 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 User image Daniel Veditz [:dveditz] 2008-02-29 16:12:03 PST
Comment on attachment 306518 [details] [diff] [review]
for 1.8

approved for, a=dveditz for release-drivers
Comment 23 User image Martijn Wargers [:mwargers] 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 User image moz_bug_r_a4 2008-03-12 19:29:12 PDT
(In reply to comment #23)
> That is what is wanted, right? 

Comment 25 User image Martijn Wargers [:mwargers] 2008-03-13 04:35:03 PDT
Ok, thanks for the info, marking verified1.8.1.13 then.
Comment 26 User image 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 User image 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 User image 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.