Closed Bug 717488 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Navigation, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox12 --- wontfix
firefox13 --- affected
firefox14 --- fixed
firefox-esr10 - wontfix

People

(Reporter: jruderman, Assigned: hsivonen)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:moderate][advisory-tracking+])

Attachments

(3 files, 1 obsolete file)

Attached file 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
Attached file stack traces
giving to hsivonen for a first look per jst. more likely resource exhaustion than a security bug?
Assignee: nobody → 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?
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.
Attached patch Paper over the problem (obsolete) — Splinter Review
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.
Oh well. I guess it's better to propagate the brokenness reason to avoid confusion about incorrect NS_ERROR_OUT_OF_MEMORY later on.
Attachment #605382 - Attachment is obsolete: true
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!
Attachment #605837 - Flags: review?(bugs)
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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.
Status: NEW → ASSIGNED
(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.
Status: ASSIGNED → NEW
(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...
Status: NEW → ASSIGNED
(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 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.
Attachment #605837 - Flags: review?(bugs) → review+
(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?
But what if the parser starts returning NS_SUCCESS_I_DID_SOMETHING!?
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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???
(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.
Whiteboard: [please leave open after inbound merge; not done here yet]
https://hg.mozilla.org/mozilla-central/rev/d07ba367be52
Whiteboard: [please leave open after inbound merge; not done here yet]
Is work going to continue with a part 2 patch in this bug? I assume so but I wanted to confirm.
(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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Depends on: 744102
Resolution: --- → FIXED
Whiteboard: [sg:moderate]
Target Milestone: --- → mozilla14
Whiteboard: [sg:moderate] → [sg:moderate][advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.