Make it safe to refcount the HTML parser's nsIStreamListener off-the-main-thread

RESOLVED FIXED in Firefox 31

Status

()

Core
HTML: Parser
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox30+ wontfix, firefox31 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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

4 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

4 years ago
Created attachment 8367867 [details] [diff] [review]
Add a separate nsIStreamListener object with thread-safe refcounting
(Assignee)

Updated

4 years ago
Blocks: 895888
(Assignee)

Updated

4 years ago
No longer blocks: 895888
(Assignee)

Comment 4

4 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 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

4 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

4 years ago
Sigh. Looks like the orange is HTML parser-related, consistent and has to be investigated. :-(

Comment 9

4 years ago
This bug is now appearing in 2.25, which is latest stable version, so I expect huge user impact.
(Assignee)

Comment 13

4 years ago
Created attachment 8406812 [details] [diff] [review]
Add a separate nsIStreamListener object with thread-safe refcounting, bitrot fixed

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 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

4 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-
But this blocks bug 938226. Don't we want that fixed everywhere? (even though it is a bit old)
(Assignee)

Comment 17

4 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.
https://hg.mozilla.org/mozilla-central/rev/547ffcbeb07a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
Can we get a beta uplift nomination here if this applies cleanly and is low risk to take?
tracking-firefox30: --- → +
Flags: needinfo?(hsivonen)
Without a response here, we're now too late for FF30 - wontfixing and it can ride from FF31.
status-firefox30: affected → wontfix
Flags: needinfo?(hsivonen)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.