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

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

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.

Comment 1

[Henri Sivonen (:hsivonen)]

Probably not worthwhile auditing. Let's just make off-the-main-thread refcounting harmless.

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Add a separate nsIStreamListener object with thread-safe refcounting

Comment 3

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=af0432bc6eb4

Comment 4

[Henri Sivonen (:hsivonen)]

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.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

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

Comment 6

[Henri Sivonen (:hsivonen)]

Let's see if the base rev for the previous try run was itself broken:
https://tbpl.mozilla.org/?tree=Try&rev=05f4454a281c

Comment 7

[Henri Sivonen (:hsivonen)]

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

Comment 8

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=120e00231364

Comment 9

[Misak Khachatryan]

This bug is now appearing in 2.25, which is latest stable version, so I expect huge user impact.

Comment 10

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=303183d12c04

Comment 11

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=9fa07db0f066

Comment 12

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=d3da69a4479d

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Add a separate nsIStreamListener object with thread-safe refcounting, bitrot fixed

The code base has changed so that the orange has gone away. Yay.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 15

[Henri Sivonen (:hsivonen)]

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

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

But this blocks bug 938226. Don't we want that fixed everywhere? (even though it is a bit old)

Comment 17

[Henri Sivonen (:hsivonen)]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/547ffcbeb07a

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get a beta uplift nomination here if this applies cleanly and is low risk to take?

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Without a response here, we're now too late for FF30 - wontfixing and it can ride from FF31.