The HTML parser should ignore incoming bytes when stopped, not just when detached.

The HTML parser should ignore incoming bytes when stopped, not just when detached, otherwise we might redundantly re-start the background parser.

Also added a few asserts to catch any future causes of the linked crash.

BUG=329603
R=abarth@chromium.org,eseidel@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165564

[eseidel, 2014-01-15 19:20:45]

From DocumentParser.h:

    // stopParsing() is used when a load is canceled/stopped.
    // stopParsing() is currently different from detach(), but shouldn't be.
    // It should NOT be ok to call any methods on DocumentParser after either
    // detach() or stopParsing() but right now only detach() will ASSERT.
    virtual void stopParsing();

I suspect that callers to appendBytes after detach are wrong.  Maybe we should
ASSERT(!isStopped()) in appendBytes to catch them as well?

I presume you ran the layout tests with a Debug build?

[oystein (OOO til 10th of July), 2014-01-15 21:07:02]

On 2014/01/15 19:20:45, eseidel wrote:
> From DocumentParser.h:
> 
>     // stopParsing() is used when a load is canceled/stopped.
>     // stopParsing() is currently different from detach(), but shouldn't be.
>     // It should NOT be ok to call any methods on DocumentParser after either
>     // detach() or stopParsing() but right now only detach() will ASSERT.
>     virtual void stopParsing();
> 
> I suspect that callers to appendBytes after detach are wrong.  Maybe we should
> ASSERT(!isStopped()) in appendBytes to catch them as well?
> 
> I presume you ran the layout tests with a Debug build?

Yep it works in debug and as we discussed offline there's situations where it
makes sense for this to be called with isStopped(), though we could probably
catch it sooner.

I just realized this breaks the XMLDocumentParser though, due to jamesr@'s
comment in DecodedDataDocumentParser.cpp:

    // This should be checking isStopped(), but XMLDocumentParser prematurely
    // stops parsing when handling an XSLT processing instruction and still
    // needs to receive decoded bytes.

So I need to either fix that, or do this somewhat differently. I'll look into
how tricky the former would be.

[oystein (OOO til 10th of July), 2014-01-16 02:09:51]

On 2014/01/15 21:07:02, Oystein wrote:
> On 2014/01/15 19:20:45, eseidel wrote:
> > From DocumentParser.h:
> > 
> >     // stopParsing() is used when a load is canceled/stopped.
> >     // stopParsing() is currently different from detach(), but shouldn't be.
> >     // It should NOT be ok to call any methods on DocumentParser after
either
> >     // detach() or stopParsing() but right now only detach() will ASSERT.
> >     virtual void stopParsing();
> > 
> > I suspect that callers to appendBytes after detach are wrong.  Maybe we
should
> > ASSERT(!isStopped()) in appendBytes to catch them as well?
> > 
> > I presume you ran the layout tests with a Debug build?
> 
> Yep it works in debug and as we discussed offline there's situations where it
> makes sense for this to be called with isStopped(), though we could probably
> catch it sooner.
> 
> I just realized this breaks the XMLDocumentParser though, due to jamesr@'s
> comment in DecodedDataDocumentParser.cpp:
> 
>     // This should be checking isStopped(), but XMLDocumentParser prematurely
>     // stops parsing when handling an XSLT processing instruction and still
>     // needs to receive decoded bytes.
> 
> So I need to either fix that, or do this somewhat differently. I'll look into
> how tricky the former would be.

Ok, some more conclusions:
1) HTMLDocumentParser::append() actually did check for isStopped() already, so
new data arriving after the parsing is stopped would only trigger a redundant
decode, and not restart the background parser.
2) for DecodedDataDocumentParser::appendBytes() to be able to properly check for
isStopped(), it should be overridden in XMLDocumentParser, and we'd either end
up with some code duplication or an even more complicated chain of calls here
(i.e. separate out the decoding from appendBytes() to decodeBytes() or
something). May or may not be worth it.
3) I've been trying to come up with a test for this situation today, but I think
the window for the crash to happen is *really* tight. I tried to provoke it by
spreading out the resource stream into single-byte packets, but no luck.

So what I'd suggest is landing the original CL again tomorrow (reverting the
revert (of the revert of the revert)), and then immediately land this one (or
merge them together, whatever is preferrable). This CL builds on top of the
other one by taking the original check for isStopped() in
HTMLDocumentParser::append() and putting it into
HTMLDocumentParser::appendBytes() where it now needs to be, i.e. going back to
the old behavior. As a side bonus we will no longer do any redundant decoding in
this situation.

[oystein (OOO til 10th of July), 2014-01-21 19:42:24]

ping, anyone :)

[abarth-chromium, 2014-01-21 21:41:00]

lgtm

[commit-bot: I haz the power, 2014-01-21 23:29:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/133173003/1

[commit-bot: I haz the power, 2014-01-22 01:18:54]

Retried try job too often on linux_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-22 01:20:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/133173003/1

[commit-bot: I haz the power, 2014-01-22 04:54:50]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-22 16:26:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/133173003/1

[commit-bot: I haz the power, 2014-01-22 19:44:27]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-22 19:46:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/133173003/1

[commit-bot: I haz the power, 2014-01-22 23:07:52]

Change committed as 165564