Closed
Bug 960519
Opened 11 years ago
Closed 11 years ago
Make it safe to refcount the HTML parser's nsIStreamListener off-the-main-thread
Categories
(Core :: DOM: HTML Parser, defect)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: hsivonen, Assigned: hsivonen)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
HTML parser crashes (bug 920725 and bug 938226) that showed up after bug 497003 strongly suggest that the code that make Necko deliver data directly to the HTML parsing thread can refcount nsHtml5StreamParser from a non-main thread, which then can result in parts of the HTML parser getting deleted prematurely.
We should find and fix the problem by auditing code.
Assignee | ||
Comment 1•11 years ago
|
||
Probably not worthwhile auditing. Let's just make off-the-main-thread refcounting harmless.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Component: Networking: HTTP → HTML: Parser
Summary: Figure out how Necko data delivery to the HTML parser thread can end up refcounting the nsHtml5StreamParser from a non-main thread → Make it safe to refcount the HTML parser's nsIStreamListener off-the-main-thread
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Comment on attachment 8367867 [details] [diff] [review]
Add a separate nsIStreamListener object with thread-safe refcounting
Does this looks sensible to you as a way to try to get rid of the crashes that showed up after bug bug 497003? I haven't actually proven that off-the-main-thread refcounting is happening, but it would explain the crashes and I don't have better explanations.
Attachment #8367867 -
Flags: feedback?(sworkman)
Comment 5•11 years ago
|
||
Comment on attachment 8367867 [details] [diff] [review]
Add a separate nsIStreamListener object with thread-safe refcounting
Review of attachment 8367867 [details] [diff] [review]:
-----------------------------------------------------------------
So, the patch looks fine to me - I don't have a better explanation for the crashes, so I think we should try it on Nightly and see if it fixes things. In theory, it shouldn't break anything. But theory can be a wonderfully problem free thing ;) f+.
Attachment #8367867 -
Flags: feedback?(sworkman) → feedback+
Assignee | ||
Comment 6•11 years ago
|
||
Let's see if the base rev for the previous try run was itself broken:
https://tbpl.mozilla.org/?tree=Try&rev=05f4454a281c
Assignee | ||
Comment 7•11 years ago
|
||
Sigh. Looks like the orange is HTML parser-related, consistent and has to be investigated. :-(
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
This bug is now appearing in 2.25, which is latest stable version, so I expect huge user impact.
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
The code base has changed so that the orange has gone away. Yay.
Attachment #8367867 -
Attachment is obsolete: true
Attachment #8406812 -
Flags: review?(bugs)
Comment 14•11 years ago
|
||
Comment on attachment 8406812 [details] [diff] [review]
Add a separate nsIStreamListener object with thread-safe refcounting, bitrot fixed
Do we need this or something similar on branches?
Attachment #8406812 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Thanks. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/547ffcbeb07a
(In reply to Olli Pettay [:smaug] from comment #14)
> Do we need this or something similar on branches?
If we want to avoid this problem, yes. If the branches don't have whatever patch it was that made the orange go away, we should probably just change the assertion expectation annotations on the branches. (I suspect bug 688580 was what made the orange go away.)
Flags: in-testsuite-
Comment 16•11 years ago
|
||
But this blocks bug 938226. Don't we want that fixed everywhere? (even though it is a bit old)
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #16)
> But this blocks bug 938226. Don't we want that fixed everywhere? (even
> though it is a bit old)
I think we want it fixed, yes. I'm not opposed to backporting to branches. I'm just saying that we ahould paper over the resulting branch oranges.
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 19•11 years ago
|
||
Marking status as we wait for Henri's input on bug 938226 about the risk of taking this at this point. It's not a topcrash on 30 but if the fix is safe (and since it's been on 31 for over a month) we could uplift before tomorrow's beta.
status-firefox30:
--- → affected
status-firefox31:
--- → fixed
Comment 20•11 years ago
|
||
Can we get a beta uplift nomination here if this applies cleanly and is low risk to take?
tracking-firefox30:
--- → +
Flags: needinfo?(hsivonen)
Comment 21•11 years ago
|
||
Without a response here, we're now too late for FF30 - wontfixing and it can ride from FF31.
Flags: needinfo?(hsivonen)
You need to log in
before you can comment on or make changes to this bug.
Description
•