280713 - OnStopRequests can confuse the parser if there is more than one parsercontext pressent

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I'm not entirely certain if this is the correct fix (or exactly how this ever
worked before), but on trunk builds on the given URL, an <iframe> under the
comic strip doesn't appear. The final two lines of the page are:
<div><iframe></div></body></html>

We reach EOF on the <iframe> and return kEOF. However, when we arrive back in
CTextToken::ConsumeCharacterData(), aScanner.IsIncremental() is PR_TRUE! Thus,
we never add the <iframe> and it disappears. My fix is, in
nsParser::ConsumeInterruptedParsing(), to set the current scanner's mIncremental
member based on whether we had reached the final chunk. I'll attach a patch shortly.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I'm slightly confused as to why this is necessary (shouldn't we have gotten an
OnStopRequest before we throw these events around?) but it fixes the issue, and
basically mimics what we do in the ::Parse(const nsAString&) function.

Can anybody shed light or give this r and sr? I think this bug is pretty bad
(we lose page content).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 173127 [details] [diff] [review]
patch v1

I think I like this patch as much as any patch. When the OnStopRequest comes,
our current context is still a script's parser context.

Johnny, the alternative to this patch is to make sure we always set the main
parser context's scanner's mIncremental member in nsParser::OnStopRequest. I'll
post another patch to that effect.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: alternative approach

This is the alternate approach. Take your pick :-).

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 173127 [details] [diff] [review]
patch v1

I *think* I like this approach better than the alternative. But I must admit
that it's been way too long since I've tried to understand what these parser
contexts really are etc. But r=jst regardless. bz should probably have a look
at this one too...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173127 [details] [diff] [review]
patch v1

sr=bzbarsky, but it sounds like some cleanup of incremental vs final chunk is
in order....

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Reopening. I still see this :-/.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixed alternative approach

This patch looks more dangerous, but it shouldn't be

What is happening is that we get a request to load a nested <script> from a
document.write(). This blocks the parser so that we can load the nested script.
However, in the meantime, we've received our stop request from the original
stream (in this case from http://www.skirtingdanger.com). So when we are
blocked and return to the event queue, we get the stop request from the stream.
However, the parser context from the original script is still on our context
list, so we were setting that context's members, and not the context that
actually had received the stop request. This patch makes sure that we always
set the correct context's members. I if()'d out the ResumeParse() call in the
case that the current context wasn't the one that received the OnStopRequest()
because it was literally just returning (I've added an assertion to this
effect). This also has the side effect of fixing bug 97886, since script parser
contexts will not ever think that they're at EOF (which is correct, if there is
an unfinished tag in a document.write(), it will be inserted into the regular
page which may contain the closing >).

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to review comments

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 173997 [details] [diff] [review]
updated to review comments

Yeah, this looks good. r=jst

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173997 [details] [diff] [review]
updated to review comments

sr=bzbarsky.  Land this either _really_ early in 1.8b2 or in 1.9a....

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Really fixed now (checked in).