608373 - Text inserted in an iframe after a <script src></script> doesn't get parsed at pageload

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Load https://bug608295.bugzilla.mozilla.org/attachment.cgi?id=486997

EXPECTED RESULTS: Both iframes should say "before scriptAFTER SCRIPT"
ACTUAL RESULTS: The one on the left just says "before script"

This bug is a spin-off from bug 608295. Quoting bug 608295 comment 4 (which describes this testcase):
=====
So this testcase shows a little of what's going on here.  It's got two iframes,
both of which get their contentDocument written at page-load-time.

The first iframe "fa" gets these contents:
> before script<script src="...do some stuff..."></script>AFTER SCRIPT

The second iframe "fb" gets these contents:
> before script<script></script>AFTER SCRIPT

The odd thing is -- in the first iframe, "AFTER SCRIPT" never visibly makes it
into the document for some reason.  (The script runs, though -- verifiable
because it adds text at the end of the page).

=====

It looks like (from the other testcase on bug 608295) the "AFTER SCRIPT" text (or any content that you want to put there) doesn't get parsed until you navigate away from the page, inside if nsDocShell::Destroy() -- specifically, in its call to Stop(nsIWebNavigation::STOP_ALL), which triggers nsHtml5TreeOpExecutor::FlushDocumentWrite() somewhere below it, which creates content & binds it to our tree.

Firefox 3.6.12pre, Opera 10.63, and Chromium all give EXPECTED RESULTS.
Today's mozilla-central nightly build gives ACTUAL RESULTS.

Filing as security-sensitive, since creating content inside of nsDocShell::Destroy seems sketchy.  (It made us violate at least one invariant in bug 608295, and I'm worried it could violate others as well.)

Comment 1

[Daniel Holbert [:dholbert]]

(oops, initially had a public bug with a private first-comment.  Fixed now)

Comment 2

[Daniel Holbert [:dholbert]]

Requesting blocking -- this is a regression in behavior, and potentially scary that we're creating new content inside of nsDocShell::Destroy.

Comment 3

[Daniel Holbert [:dholbert]]

Regression range:
 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=d6bb0f9e9519

This includes "Bug 373864 - Enable the HTML5 parser by default", which is just a pref-flip for "htm5.enabled".

If I flip this pref back in today's nightly (and restart Firefox for good measure), this bug goes away.  So, regression from bug 373864.

Comment 4

[Johnny Stenback (:jst)]

Henri, not sure if this is related to the HTML5 parser or not, but can you have a look and let me know if this is something someone else should work on?

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to comment #0)
> The odd thing is -- in the first iframe, "AFTER SCRIPT" never visibly makes it
> into the document for some reason.  (The script runs, though -- verifiable
> because it adds text at the end of the page).

Not sure yet why that happens. Very odd.

> Filing as security-sensitive, since creating content inside of
> nsDocShell::Destroy seems sketchy.  (It made us violate at least one invariant
> in bug 608295, and I'm worried it could violate others as well.)

This happens because I've implemented document.close() assuming it gets called from the script and that it has to show a certain DOM state to the script when returning. However, there's a non-script caller here:
void
nsHTMLDocument::StopDocumentLoad()
{
  // If we're writing (i.e., there's been a document.open call), then
  // nsDocument::StopDocumentLoad will do the wrong thing and simply terminate
  // our parser.
  if (mWriteState != eNotWriting) {
    Close();
  } else {
    nsDocument::StopDocumentLoad();
  }
}

So bug 332896 needs refixing in a way that doesn't call the implementation document.close() that's meant for scripts.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: WIP: Don't call document.close() when stopping a load

This patch fixes the scary white-box behavior but doesn't fix the odd black-box behavior.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Avoid calling .close() from C++, flush ops when exhausting the stream in the script-created parser case

I'm shocked this bug wasn't caught earlier.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If RemoveWyciwygChannel can trigger onload, then it can trigger a document.open() call under itself, no?  So changing mWriteState after that is rather suspect...

Do we need to make sure we block the onload firing until after we're done with calling into the superclass here?

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to comment #8)
> If RemoveWyciwygChannel can trigger onload, then it can trigger a
> document.open() call under itself, no?  So changing mWriteState after that is
> rather suspect...

Good point. I'll fix.

> Do we need to make sure we block the onload firing until after we're done with
> calling into the superclass here?

At least the old code doesn't do so.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The old code also doesn't touch any of our members after onload might have fired, no?

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Avoid calling .close() from C++, flush ops when exhausting the stream in the script-created parser case, v2

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 490540 [details] [diff] [review]
Avoid calling .close() from C++, flush ops when exhausting the stream in the script-created parser case, v2

I'm not sure if the onload blocking/unblocking is necessary, but I added it as suggested just in case.

Is the commit message appropriate for landing?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490540 [details] [diff] [review]
Avoid calling .close() from C++, flush ops when exhausting the stream in the script-created parser case, v2

The "If all other requests" comment should probably just go away.

The checkin comment looks fine.

Comment 14

[Henri Sivonen (:hsivonen)]

Thanks. Landed without the "If all other requests" comment.
http://hg.mozilla.org/mozilla-central/rev/8f5e0944335d