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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({crashreportid, hang})

Trunk
mozilla2.0b7
crashreportid, hang
Points:
---

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
I've got a trivial patch for this - posting in a few minutes.
(Assignee)

Comment 2

8 years ago
Created attachment 478403 [details] [diff] [review]
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.
Attachment #478403 - Flags: review?(jst)
(Assignee)

Updated

8 years ago
Assignee: nobody → dholbert
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 years ago
(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.)
(Assignee)

Comment 4

8 years ago
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...
blocking2.0: --- → ?
blocking2.0: ? → beta7+
(Assignee)

Comment 5

8 years ago
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)
Attachment #478403 - Flags: review?(bzbarsky)

Updated

8 years ago
Attachment #478403 - Flags: review?(jst) → review+
(Assignee)

Updated

8 years ago
Attachment #478403 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

8 years ago
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
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
(Assignee)

Updated

7 years ago
Blocks: 656244
You need to log in before you can comment on or make changes to this bug.