Closed Bug 608373 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dholbert, Assigned: hsivonen)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [sg:critical?])

Attachments

(1 file, 2 obsolete files)

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.)
(oops, initially had a public bug with a private first-comment.  Fixed now)
Group: core-security
Requesting blocking -- this is a regression in behavior, and potentially scary that we're creating new content inside of nsDocShell::Destroy.
blocking2.0: --- → ?
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.
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?
Assignee: nobody → hsivonen
blocking2.0: ? → betaN+
(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.
Status: NEW → ASSIGNED
Whiteboard: [sg:critical?]
This patch fixes the scary white-box behavior but doesn't fix the odd black-box behavior.
I'm shocked this bug wasn't caught earlier.
Attachment #490054 - Attachment is obsolete: true
Attachment #490070 - Flags: review?(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?
(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.
The old code also doesn't touch any of our members after onload might have fired, no?
Attachment #490070 - Flags: review?(bzbarsky) → review-
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?
Attachment #490540 - Flags: review?(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.
Attachment #490540 - Flags: review?(bzbarsky) → review+
Thanks. Landed without the "If all other requests" comment.
http://hg.mozilla.org/mozilla-central/rev/8f5e0944335d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.