717488 - "ASSERTION: Uh, mDocument doesn't match the current inner window document!" with nearNativeStackLimit, sync XHR

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Script global object not ready: '!mDocShell || GetDocument()->GetScriptGlobalObject()', file parser/html/nsHtml5TreeOpExecutor.h, line 157

###!!! ASSERTION: Uh, mDocument doesn't match the current inner window document!: '!GetCurrentInnerWindow() || GetCurrentInnerWindow()->GetExtantDocument() == mDocument', file dom/base/nsGlobalWindow.cpp, line 1877

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Daniel Veditz [:dveditz]]

giving to hsivonen for a first look per jst. more likely resource exhaustion than a security bug?

Comment 3

[Henri Sivonen (:hsivonen)]

The assertion fires for data:text/html,1. That is, inserting a new iframe and spinning the event loop makes OnStartRequest for the new iframe fire before the document has been set up the usual way.

The HTML parser is prepared to handle OnDataAvailable and executor flushes when sync XHR is spinning the event loop, but it's not prepared for OnStartRequest firing prematurely.

Maybe the script global object for the iframe is waiting to be set up from a script runner that won't run until the script that inserted the iframe and started XHR finishes?

Comment 4

[Henri Sivonen (:hsivonen)]

On the HTML parser side, what could be done here is calling MarkAsBroken() instead of asserting here:
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOpExecutor.h#156

Still need a real fix for the iframe/docshell part, though.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Paper over the problem

This patch doesn't make Gecko behave correctly, but hopefully this makes Gecko behave less dangerously. I think we should at least take this patch.

I'm not quite happy with marking the parser broken for non-OOM reasons, because all code that checks if the parser is broken claims OOM if marked broken. OTOH, propagating the right brokenness reason seems like an overkill.

I still need to understand how OnStartRequest and script global object initialization can change order.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Paper over the problem, propagate the brokenness reason

Oh well. I guess it's better to propagate the brokenness reason to avoid confusion about incorrect NS_ERROR_OUT_OF_MEMORY later on.

Comment 7

[Johnny Stenback (:jst)]

Henri, any ideas what this could lead to? We're having a hard time figuring out what sg: rating to put on this bug, so any help would be appreciated!

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #7)
> Henri, any ideas what this could lead to? We're having a hard time figuring
> out what sg: rating to put on this bug, so any help would be appreciated!

I don't know what this could lead to. Lacking a script global object is the normal situation for documents loaded via XHR, so it's possible that this isn't really a security bug and instead just prevents some scripts from compiling.

But I don't really know how not having a script global object interacts with *having* a docshell.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> I still need to understand how OnStartRequest and script global object
> initialization can change order.

nsDocument::MaybeInitializeFinalizeFrameLoaders looks like a suspect for varying when script global object gets set on an iframed doc.

Still unclear how the stream OnStartRequest doesn't vary together with that.

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> (In reply to Henri Sivonen (:hsivonen) from comment #5)
> > I still need to understand how OnStartRequest and script global object
> > initialization can change order.
> 
> nsDocument::MaybeInitializeFinalizeFrameLoaders looks like a suspect for
> varying when script global object gets set on an iframed doc.

That's removing the script global object of the previous doc.

I don't find anything of interest on the script global object initialization path.

I wonder if sometimes the removal of the previous script global object gets deferred and the deferred removal overwrites the new script global object with null...

Comment 11

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

(In reply to Henri Sivonen (:hsivonen) from comment #8)
> I don't know what this could lead to. Lacking a script global object is the
> normal situation for documents loaded via XHR, so it's possible that this
> isn't really a security bug and instead just prevents some scripts from
> compiling.
Documents without script global or script handling object used to be security problems.
But nowadays nsDOMClassInfo shouldn't allow creating wrappers for nodes in such documents.

Henri, when the assertion fires, is nsIDocument::mHasHadScriptHandlingObject true?
Since I would feel better if it was.

Comment 12

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

Comment on attachment 605837 [details] [diff] [review]
Paper over the problem, propagate the brokenness reason

> nsHtml5Parser::Parse(const nsAString& aSourceBuffer,
>                      void* aKey,
>                      const nsACString& aContentType,
>                      bool aLastCall,
>                      nsDTDMode aMode) // ignored
> {
>-  if (mExecutor->IsBroken()) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>+  nsresult rv;
>+  if ((rv = mExecutor->IsBroken())) {
>+    return rv;
Could you use NS_SUCCEEDED().


>+nsHtml5TreeOpExecutor::WillBuildModel(nsDTDMode aDTDMode) {
{ should be in the next line

>     bool                          mCallContinueInterruptedParsingIfEnabled;
> 
>     /**
>-     * True if this parser should refuse to process any more input.
>-     * Currently, the only way a parser can break is if it drops some input
>-     * due to a memory allocation failure. In such a case, the whole parser
>-     * needs to be marked as broken, because some input has been lost and
>-     * parsing more input could lead to a DOM where pieces of HTML source
>+     * Non-NS_OK if this parser should refuse to process any more input.
>+     * For example, the parser needs to be marked as broken if it drops some
>+     * input due to a memory allocation failure. In such a case, the whole
>+     * parser needs to be marked as broken, because some input has been lost
>+     * and parsing more input could lead to a DOM where pieces of HTML source
>      * that weren't supposed to become scripts become scripts.
>+     *
>+     * Since NS_OK is actually 0, zeroing operator new takes care of
>+     * initializing this.
>      */
>-    bool                          mBroken;
>+    nsresult                       mBroken;
1 extra space before mBroken

r=me, but please check the value of mHasHadScriptHandlingObject.
If it is false, we should probably mark it true somewhere.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #12)
> >+  if ((rv = mExecutor->IsBroken())) {
> >+    return rv;
> Could you use NS_SUCCEEDED().

Even though NS_OK is known to be 0, any non-NS_OK value means the parser is broken and NS_SUCCEEDED does a bitwise operation that's useless here?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

But what if the parser starts returning NS_SUCCESS_I_DID_SOMETHING!?

Comment 15

[Henri Sivonen (:hsivonen)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #14)
> But what if the parser starts returning NS_SUCCESS_I_DID_SOMETHING!?

As a marker that the parser is broken???

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 605837 [details] [diff] [review]
> Paper over the problem, propagate the brokenness reason
> 
> > nsHtml5Parser::Parse(const nsAString& aSourceBuffer,
> >                      void* aKey,
> >                      const nsACString& aContentType,
> >                      bool aLastCall,
> >                      nsDTDMode aMode) // ignored
> > {
> >-  if (mExecutor->IsBroken()) {
> >-    return NS_ERROR_OUT_OF_MEMORY;
> >+  nsresult rv;
> >+  if ((rv = mExecutor->IsBroken())) {
> >+    return rv;
> Could you use NS_SUCCEEDED().

I think you meant NS_FAILED. I used NS_FAILED, though I don't think it adds value (adds a useless and operation).

> >+nsHtml5TreeOpExecutor::WillBuildModel(nsDTDMode aDTDMode) {
> { should be in the next line

Fixed.

> >     bool                          mCallContinueInterruptedParsingIfEnabled;
> > 
> >     /**
> >-     * True if this parser should refuse to process any more input.
> >-     * Currently, the only way a parser can break is if it drops some input
> >-     * due to a memory allocation failure. In such a case, the whole parser
> >-     * needs to be marked as broken, because some input has been lost and
> >-     * parsing more input could lead to a DOM where pieces of HTML source
> >+     * Non-NS_OK if this parser should refuse to process any more input.
> >+     * For example, the parser needs to be marked as broken if it drops some
> >+     * input due to a memory allocation failure. In such a case, the whole
> >+     * parser needs to be marked as broken, because some input has been lost
> >+     * and parsing more input could lead to a DOM where pieces of HTML source
> >      * that weren't supposed to become scripts become scripts.
> >+     *
> >+     * Since NS_OK is actually 0, zeroing operator new takes care of
> >+     * initializing this.
> >      */
> >-    bool                          mBroken;
> >+    nsresult                       mBroken;
> 1 extra space before mBroken

Fixed.
 
> r=me, 

Thank you. Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d07ba367be52

> but please check the value of mHasHadScriptHandlingObject.
> If it is false, we should probably mark it true somewhere.

The trunk has changed so that I have trouble catching the bug in a debugger again. This bug needs more investigation but at least we have some mitigation now.

Comment 17

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/mozilla-central/rev/d07ba367be52

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Is work going to continue with a part 2 patch in this bug? I assume so but I wanted to confirm.

Comment 19

[Henri Sivonen (:hsivonen)]

(In reply to Al Billings [:abillings] from comment #18)
> Is work going to continue with a part 2 patch in this bug? I assume so but I
> wanted to confirm.

In principle, yes, hopefully, but not with an immediate top priority. I suppose we could spin part 2 into a different bug number in order to better manage bug state.

Comment 20

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #19)
> I
> suppose we could spin part 2 into a different bug number in order to better
> manage bug state.

To make things clearer, I'm closing this bug as FIXED since the consequences were mitigated. I filed bug 744102 about fixing the root cause.