599497 - Hang inside of [@ nsParser::Tokenize] while loading SVG-as-a-background-image

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

When loading the first testcase dbaron gave in bug 597778 comment 0...
http://test.csswg.org/suites/css2.1/20100917/xhtml1/background-intrinsic-001.xht
... Firefox hangs, depending on the circumstances.  (I've reproduced it most successfully in my debug build, as well as an an opt build **with my normal browsing profile** (but not opt build + empty profile))

Here are two sample crash reports, from running "kill -11 [pid]" to stop the hanging:
bp-f284074b-49fe-4853-a5f8-e5a0f2100924
bp-d01cca84-13bc-48fa-8edf-a78d62100924

Comment 1

[Daniel Holbert [:dholbert]]

I've got a trivial patch for this - posting in a few minutes.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix

So the problem here is that we were entering ContinueInterruptedParsing with the expectation that more data was coming, when it might not be.

In particular, in this part

1764 nsParser::ContinueInterruptedParsing()
1765 {
[...]
1789 
1790   PRBool isFinalChunk = mParserContext &&
1791                         mParserContext->mStreamListenerState == eOnStop;
http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsParser.cpp#1790
...we end up setting isFinalChunk to *false*, when it should be true, because our state is "eOnDataAvail".

And our state is "eOnDataAvail" because we haven't called OnStopRequest on the parser yet. (though we're about to, up one stack-level)

This patch makes us call OnStopRequest on the parser right before we call ContinueInterruptedParsing, instead of the other way around.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> ...we end up setting isFinalChunk to *false*, when it should be true, because
> our state is "eOnDataAvail".

Sorry, I said that in a confusing way. What I meant to say is:
We end up setting isFinalChunk to *false*, because our state is "eOnDataAvail".  (And then we hang, because we try to read, get 0 bytes, and then keep trying & getting nothing.)
However, we'd *like* to have isFinalChunk set to true, because then we'll be OK with reading 0 bytes, and we won't hang.  (And if we call OnStopRequest earlier, as we do in this patch, then our state ends up being eOnStop, and things work out like we want them to.)

Comment 4

[Daniel Holbert [:dholbert]]

I verified that this passes the SVG-as-an-image reftests, btw.

If possible, I'd like this to make it into b7 -- that'll be the first beta to ship with SVG-as-an-image, and as it stands, this could be a nasty hang affecting anyone who's trying out that feature. (I'll be doing a hacks blog post soon, and I'd hate for that to accidentally DOS anyone who views it. :))

Given that there are still 25 open b7-blockers, I'm hoping maybe there's still time to slip this one-liner in...

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 478403 [details] [diff] [review]
fix

Adding bz as an alternate reviewer, in case he can get to this before jst, because I'd ideally like to land this tomorrow morning if the tree cooperates. :)  (after a sanity-check TryServer run)

Comment 6

[Daniel Holbert [:dholbert]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/ec28605a1fc8

I also added an
>+    if (!parser->IsComplete()) {
check, around the
>+      parser->ContinueInterruptedParsing();
call, to fix some assertions that showed up on TryServer[1] (but hadn't showed up on my local machine in comment 4, for whatever reason).  I think these assertions were from ContinueInterruptedParsing getting confused when OnStopRequest had already handled everything (so there was nothing to "continue").  In any case,  I verified that the above if-check fixed the assertions, in another TryServer run.

[1] e.g. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1285378018.1285379383.20808.gz