Closed Bug 1353660 Opened 3 years ago Closed 3 years ago

Crash in nsXPCWrappedJS::Release for ~HttpChannelChild

Categories

(Core :: Networking: HTTP, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: ting, Assigned: froydnj)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-3f9581fb-780e-4d01-ae63-ab4d52170404.
=============================================================
Top #38 of Nightly 20170402030202 on Windows, 5 crashes from 4 installations.
Might cause by destructing an inherited member on base class of HttpChannelChild.
https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/ds/nsHashPropertyBag.h#32

`nsInterfaceHashtable<nsStringHashKey, nsIVariant>` might not be thread-safe to delete since nsIVariant can be some main-thread-only object. However, we made nsHashPropertyBag implement thread-safe refcounting. :(

@nfroiyd do you think this is some we should fix in xpcom/ds?
Flags: needinfo?(nfroyd)
Wow, there are a lot of Thunderbird crashes because of this.

There are two ways we can fix this:

1) Have HttpChannelChild proxy the destruction of the hashtable to the main thread.

2) Have nsHashPropertyBag proxy the destruction of the hashtable to the main thread.

The latter seems better.  Do you have cycles to fix this, or shall I?
Flags: needinfo?(nfroyd)
I only have time to do (1) recently, will not have time to try (2). 
It'll be really helpful if you can ,or find someone familiar with this code to, take (2) and fix the whole thing.
Flags: needinfo?(nfroyd)
We need this because the stored values in the hash table may themselves
be main-thread only objects, and destroying them off the main thread
will cause crashes.

I think that it's better to fix it here once and for all than to just fix it in
the HTTP stack...even though I think the HTTP stack is the only thing that
really cares about thread-safetyness of nsHashPropertyBag.  If we're going to
have threadsafe refcounting on nsHashPropertyBag, though, we might as well make
sure the class itself is resilient to multiple threads.
Attachment #8854885 - Flags: review?(continuation)
Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

Review of attachment 8854885 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsHashPropertyBag.cpp
@@ +268,5 @@
>  NS_INTERFACE_MAP_END
>  
> +/*
> + * We need to ensure that the hashtable is destroyed on the main thread, as
> + * the nsIVariant values may be (almost certainly are) main-thread only

Maybe drop the "may be" part of this? IMO it is a little confusing to read like this.

@@ +282,5 @@
> +
> +  NS_IMETHODIMP
> +  Run()
> +  {
> +    // Ensure destruction happens on the main thread.

Maybe assert we are on the main thread instead of the comment?
Attachment #8854885 - Flags: review?(continuation) → review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #3)
> I only have time to do (1) recently, will not have time to try (2). 
> It'll be really helpful if you can ,or find someone familiar with this code
> to, take (2) and fix the whole thing.

Done!
Flags: needinfo?(nfroyd)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b093fc20d119
proxy destruction of nsHashPropertyBag's hash table to the main thread; r=mccr8
Thanks @froiydnj! Really appreciate your promptly support.
https://hg.mozilla.org/mozilla-central/rev/b093fc20d119
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please nominate this for Aurora approval when you get a chance. We can take a look at ESR52 during the next cycle.
Flags: needinfo?(nfroyd)
Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

Approval Request Comment
[Feature/Bug causing the regression]: long-standing bug.
[User impact if declined]: crashes, particularly with addons that heavily manipulate the network stack.
[Is this code covered by automated tests?]: The code is certainly exercised in automated tests, but the crashes that it would fix are not covered.
[Has the fix been verified in Nightly?]: No.  Not really clear how to verify.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: We're just moving the destruction of an object to a different thread.
[String changes made/needed]: None.
Flags: needinfo?(nfroyd)
Attachment #8854885 - Flags: approval-mozilla-aurora?
Thunderbird definitely wants this, since the majority of crashes with this signature are coming from Thunderbird in various versions.  I don't know how Thunderbird branching works, but even if this doesn't make it into ESR52 (which would be unfortunate), Thunderbird should cherry-pick this fix anyway.
Flags: needinfo?(jorgk)
Yes, I can uplift this to out THUNDERBIRD_52_VERBRANCH if required. That's a branch on mozille-esr52. Thanks for letting me know.
s/out/our/.
Flags: needinfo?(jorgk)
Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes crashes with addons.  Thunderbird would also appreciate having this in ESR52 and not having to maintain a separate patch for it.
User impact if declined: Crashes.
Fix Landed on Version: 55, and 54 if aurora is approved.
Risk to taking this patch (and alternatives if risky): Very small.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8854885 - Flags: approval-mozilla-esr52?
Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

We can take this and see if the crashes are decreased. Aurora54+.
Attachment #8854885 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

There has been one instance of this crash on esr52.1.0 in past 7 days. I don't see the benefit of taking this uplift in ESR52.2.
Attachment #8854885 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
You need to log in before you can comment on or make changes to this bug.