1544127 - Crash in [@ mozilla::net::CookieSettings::Release] and [@ mozilla::net::LoadInfo::~LoadInfo]

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-02ca13fa-8b0a-4fca-956a-1e6660190410.

Top 10 frames of crashing thread:

0 xul.dll unsigned long mozilla::net::CookieSettings::Release netwerk/cookie/CookieSettings.cpp:246
1 xul.dll mozilla::net::LoadInfo::~LoadInfo netwerk/base/LoadInfo.h:139
2 xul.dll mozilla::net::LoadInfo::Release netwerk/base/LoadInfo.cpp:616
3 xul.dll mozilla::net::HttpBaseChannel::~HttpBaseChannel netwerk/protocol/http/HttpBaseChannel.cpp:254
4 xul.dll void mozilla::net::nsHttpChannel::~nsHttpChannel netwerk/protocol/http/nsHttpChannel.cpp:350
5 xul.dll unsigned long mozilla::dom::ExternalHelperAppParent::Release xpcom/base/nsSystemInfo.cpp:1050
6 xul.dll mozilla::dom::DeferredFinalizerImpl<nsISupports>::DeferredFinalize dom/bindings/BindingUtils.h:2695
7 xul.dll mozilla::IncrementalFinalizeRunnable::ReleaseNow xpcom/base/CycleCollectedJSRuntime.cpp:1253
8 xul.dll nsresult mozilla::IncrementalFinalizeRunnable::Run xpcom/base/CycleCollectedJSRuntime.cpp:1289
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1179

this crash signature is starting to show up in low volume in pre-release versions of firefox 67. the top frames of the stacks point to code recently touched in bug 1525245, so i assume that could be the regressor.

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Baku, seems like a regression from bug 1525245, can you please take a look? It's already in Beta.

Comment 2

[Daniel Veditz [:dveditz]]

Calling it sec-critical because it's a stack buffer overrun, but maybe if it's a race condition we can downgrade it to sec-high.

Comment 3

[Daniel Veditz [:dveditz]]

The stack buffer overrun was 3 crashes from the same install. The rest look like more typical UAF crashes so maybe the crash reporter just got confused about the cause (UAF read was from a non-crashing address, but it was still bogus data and led to writing on the stack).

Comment 4

[Andrea Marchesini [:baku]]

We have several similar crashes with signature LoadInfo::~LoadInfo(). I don't think the bug is in CookieSettings, but elsewhere in the LoadInfo, and, during the releasing of the object, we crash in different points. See this list:

https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Anet%3A%3ALoadInfo%3A%3A~LoadInfo&date=%3E%3D2019-05-21T09%3A16%3A00.000Z&date=%3C2019-05-28T09%3A16%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports

Comment 5

[Andrea Marchesini [:baku]]

Valentin, does the stack-trace look familiar to you?

Comment 6

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

I am not familiar with any such issues in LoadInfo.
Indeed, there seem to be some crashes prior to bug 1525245 landing, so it's probably not at fault here.
About a third of the crashes I've looked at have ExternalHelperAppParent on the stack, but I haven't seen any obvious bug in the implementation.
I'm wondering if a racy release of LoadInfo may be happening off-main-thread (meaning it's usually on the main thread, but sometimes off main thread), and that is causing the main-thread refcount to be wrong?

Comment 7

[Liz Henry (:lizzard)]

Baku, can you investigate or help find another owner for this issue? It is likely too late to fix in 68 at this point.

Comment 8

[Andrea Marchesini [:baku]]

I suspect that the problem is not in LoadInfo or in CookieSettings. It's more likely to be in ExternalHelperAppParent or nsHttpChannel.

I found many crash reports like this: https://crash-stats.mozilla.org/report/index/3a9c2cc1-f62c-4389-b903-797540190630
I updated the signature to include [@ mozilla:net::nsHttpChannel::~nsHttpChannel]
They all contain 'ExternalHelperAppParent::Release()'.

Can somebody from the necko team take a look?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Nhi, is there anybody on the Necko team with cycles to take a look at this?

Comment 10

[Nhi Nguyen (:nhi)]

Michal, could you take a look? Thanks!

Comment 11

[Michal Novotny [:michal]]

(In reply to PTO until September 9th from comment #6)

I'm wondering if a racy release of LoadInfo may be happening off-main-thread (meaning it's usually on the main thread, but sometimes off main thread), and that is causing the main-thread refcount to be wrong?

LoadInfo doesn't have thread-safe refcounting. We would get an assertion if it would be addrefed or released on a different thread. But there might be edge cases which do not happen on the testing infrastructure. We should IMO make it thread-safe and see what it does with crash stats.

Comment 12

[Michal Novotny [:michal]]

Attachment: Bug 1544127 - Make LoadInfo refcounting thread-safe, r=mayhemer

Comment 13

[Honza Bambas (:mayhemer)]

(This somewhat suggest to make the assertions for thread safety counter DIAGNOSTIC)

Comment 14

[Michal Novotny [:michal]]

Comment on attachment 9078740 [details]
Bug 1544127 - Make LoadInfo refcounting thread-safe, r=mayhemer

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's very hard or impossible. We're not even sure that non thread-safe refcounting is causing this crash.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: It cannot cause regression.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/15f6b0cf4eb099b2d192f190939ef54020653f5c
https://hg.mozilla.org/mozilla-central/rev/15f6b0cf4eb0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta/ESR68/ESR60 approval when you get a chance.

Comment 18

[Michal Novotny [:michal]]

It seems that the patch didn't fix the crash: https://crash-stats.mozilla.org/report/index/be45debf-2217-47d7-b2f7-f45980190723

I think we should keep the patch in the tree, but it's not worth uplifting it.

Comment 19

[Honza Bambas (:mayhemer)]

I think we should back the patch out, atomic refcounting has its overhead and clearly here it doesn't solve anything and non-atomic refcounter never triggered an assertion failure.

Comment 20

[Michal Novotny [:michal]]

Attachment: Bug 1544127 - Backout changeset 15f6b0cf4eb0, r=mayhemer

Comment 21

[Michal Novotny [:michal]]

Comment on attachment 9084170 [details]
Bug 1544127 - Backout changeset 15f6b0cf4eb0, r=mayhemer

Security Approval Request

• How easily could an exploit be constructed based on the patch?: n/a
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: n/a
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: n/a
• How likely is this patch to cause regressions; how much testing does it need?: this is just a backout of the commit that didn't fix the bug

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9084170 [details]
Bug 1544127 - Backout changeset 15f6b0cf4eb0, r=mayhemer

Approval given.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/762fda8f1dc090bb4c923cb49bec15eb23e94091
https://hg.mozilla.org/mozilla-central/rev/762fda8f1dc0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

IIUC, this bug isn't actually fixed. We just backed out the first patch since it didn't help. Also resetting the ESR tracking flags since it's not looking likely that we'll get a fix in time for this cycle.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Too late for 69 at this point with RC coming next week.

Comment 26

[Daniel Veditz [:dveditz]]

The original [@mozilla::net::CookieSettings::Release] crash is gone in Fx70.0a1. The LoadInfo::~LoadInfo crashes are still here, but they also predate the supposed Fx67 regressor that put the blame on "DOM: Security". This doesn't seem to be the right component

Comment 27

[Honza Bambas (:mayhemer)]

I was talking to Christian about this bug and he may try to run fuzzing on this. Christian, if this is too complicated, feel free to bounce.

Comment 28

[Christian Holler (:decoder)]

(In reply to Honza Bambas (:mayhemer) from comment #27)

I was talking to Christian about this bug and he may try to run fuzzing on this. Christian, if this is too complicated, feel free to bounce.

I think fuzzing could only be effective here, if we knew more about the environment/conditions under which this crash can be triggered at all. The new Necko fuzzing targets did not hit any issues in or around LoadInfo. Without having more specific information about what an additional fuzzing target should do (e.g. "multiple HTTP requests with Cookies of Type X set", just brainstorming), I don't think this is actionable.

Comment 29

[Honza Bambas (:mayhemer)]

P2 for hard to act on.

Comment 30

[Honza Bambas (:mayhemer)]

As the measure #1 - I'm going to use a build with enabled refcounter t-s checks for few days. Let's see if something catches.

Comment 31

[Honza Bambas (:mayhemer)]

Adding Boris and Nika. Please, more for an advice where to look or how to bisect this then a direct help to figure this out.

According the symptoms (random crashes at random frames) this first reminded me of Illusion of atomic reference counting - mayhemer's blog - random crashes on random addresses and frames.

But, we are crashing on the main thread/any process (all c-rs I have checked) only. The crash for nsHttpChannel::~nsHttpChannel sig happens only from the cycle collector. There is also ~LoadInfo crash from CC. One is coming from the IPC code, but it may be just because CC has won the last safe Release().

Hence, I suspect CC double releases some objects, specifically nsHttpChannel.

Current status:

• I walked few distinctive crash reports and was inspecting the code (callers) for a chance of use of some badly assigned raw ptr or similar mistakes, nothing found (but to spot this just like that is hard anyway)
• the custom nightly build with a forced thread safety refcnt checking didn't catch a single misuse; going to double check the build was really doing what I wanted (if not, fix and use again)
• filed and worked on bug 1582228 that would try to detect at runtime a concurrent access on RefPtr/nsCOMPtr members and globals; turned out not that easy to be implemented, currently stalled (would need more time to finish and involve more people, but could find and prevent number of mistakes potentially leading to UAFs)

Comment 32

[Honza Bambas (:mayhemer)]

Olli, please see comment 31.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hence, I suspect CC double releases some objects, specifically nsHttpChannel.

That seems ... pretty unlikely. Especially since nsHttpChannel is not cycle-collected, right?

I don't see how ExternalHelperAppParent::Release would lead to nsHttpChannel::~nsHttpChannel being called, as some of the stacks claim. Am I just missing something there?

Have we tried adding threadsafety release asserts in ExternalHelperAppParent refcounting, just to see whether that could be an issue? Have we tried adding such asserts in LoadInfo refcounting?

Comment 34

[Liz Henry (:lizzard)]

Too late to fix in 70 at this point but we could still potentially take a patch in 71.

Comment 36

[Honza Bambas (:mayhemer)]

Adding the stalled keyword, there is no clear direction to figure this out at the moment.

Boris, I'm not sure I follow your comment 33.

this stack:

mozilla::net::nsHttpChannel::~nsHttpChannel()
void mozilla::net::nsHttpChannel::~nsHttpChannel()
unsigned long mozilla::net::DataChannelChild::Release()
mozilla::dom::DeferredFinalizerImpl<nsISupports>::DeferredFinalize(unsigned int, void*)
mozilla::IncrementalFinalizeRunnable::ReleaseNow(bool)
nsresult mozilla::IncrementalFinalizeRunnable::Run()
nsThread::ProcessNextEvent(bool, bool*)

shows that we call IncrementalFinalizeRunnable::ReleaseNow, ending up in DeferredFinalizerImpl, which pops pointers and calls Release() on the affected nsHttpChannel object (I believe it's code optimization/sharing why we see DataChannelChild::Release and not nsHttpChannel::Release). This is probably an implementation detail of CC, but I don't know its guts that well to understand what this code really does. How did nsHttpChannel got into the mDeferredFinalizeFunctions array of IncrementalFinalizeRunnable? As I understand, this is grabbing CycleCollectedJSRuntime.mDeferredFinalizerTable.

Re refcounter checking: I tried to run a build with the following patch (on Win) Fri, Sep 13, 13:50:20, no crashes caught :( Not sure if this change can really detect something on official builds:

--- a/xpcom/base/nsDebug.h
+++ b/xpcom/base/nsDebug.h
@@ -308,18 +308,17 @@ inline void MOZ_PretendNoReturn() MOZ_PR
 
 #define NS_ENSURE_STATE(state) NS_ENSURE_TRUE(state, NS_ERROR_UNEXPECTED)
 
 #define NS_ENSURE_NO_AGGREGATION(outer) \
   NS_ENSURE_FALSE(outer, NS_ERROR_NO_AGGREGATION)
 
 /*****************************************************************************/
 
-#if (defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING))) && \
-    !defined(XPCOM_GLUE_AVOID_NSPR)
+#if !defined(XPCOM_GLUE_AVOID_NSPR)
 #  define MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED 1
 #endif
 
 #ifdef MOZILLA_INTERNAL_API
 void NS_ABORT_OOM(size_t aSize);
 #else
 inline void NS_ABORT_OOM(size_t) { MOZ_CRASH(); }
 #endif

Comment 37

[Honza Bambas (:mayhemer)]

Reverting the crash sig. (No longer detecting collisions....?)

Comment 38

[Boris Zbarsky [:bzbarsky]]

I believe it's code optimization/sharing why we see DataChannelChild::Release and not nsHttpChannel::Release

OK, then ExternalHelperAppParent::Release is probably also wrong in the other stacks here?

How did nsHttpChannel got into the mDeferredFinalizeFunctions array of IncrementalFinalizeRunnable

Probably via a call to mozilla::DeferredFinalize. This can happen in various ways, but the most likely one for nsHttpChannel is via XPCWrappedNative::FlatJSObjectFinalized. That is, someone touched an HTTP channel from JS, then the JS reflector object got GCed, and we scheduled a deferred Release call, because we don't want to be running C++ destructors during the finalization phase of JS GC (and that's because some C++ destructors can call into JS, which is really not desirable while running JS object finalizers). There's no cycle collection involved here, apart from the fact that the cycle collector also uses this "call Release off a runnablemachinery and that the implementation ofmozilla::DeferredFinalizehappens to live in the per-threadCycleCollectedJSRuntime` struct. It could just as easily use some other thread singleton and work exactly the same.

I tried to run a build with the following patch

If ExternalHelperAppParent is not actually involved, so we're just looking at LoadInfo's refcounting, right?

MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED would, I think catch things, but my point was that we should add release-enabled asserts in LoadInfo refcounting and ship those asserts to our actual release population, so we can see whether those are getting hit. I don't think we want to turn on MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED in general on release...

Comment 39

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

bz answered here already. CC doesn't really seem to be involved.
(DeferredFinalizerImpl isn't a CC thing, it has been added to avoid calling Release synchronously when GC calls finalizer on GCed objects)

Comment 40

[Nika Layzell [:nika] (ni? for response)]

I don't have particular familiarity with this code, and I don't think I can add much meaningful here than is already mentioned above.

Comment 41

[Honza Bambas (:mayhemer)]

Thanks everyone, and specially bz, for answers.

I'll file a separate, open bug to do all-time enabled release-assertion for threadsafety of refcnt and land for a while on nightly. but I honestly doubt it will catch anything related to this bug.

I am back at suspecting the illusion bug, reassigning a comptr/refptr holding nsHttpChannel on more than one thread. back to bug 1582228.

Comment 42

[Honza Bambas (:mayhemer)]

Filed bug 1590761

Comment 43

[Pascal Chevrel (relman team) -> :pascalc]

Wontfix 71 as the bug is stalled and we are past the mid-beta cycle

Comment 44

[Nathan Froyd [:froydnj]]

needinfo'ing myself to check these minidumps for potential bad memory when I'm back at a real computer.

Comment 45

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #44)

needinfo'ing myself to check these minidumps for potential bad memory when I'm back at a real computer.

I looked at a dozen or so minidumps from:

https://crash-stats.mozilla.org/search/?release_channel=release&signature=%3DnsTArray_Impl%3CT%3E%3A%3A~nsTArray_Impl%20%7C%20mozilla%3A%3Anet%3A%3ACookieSettings%3A%3A~CookieSettings&product=Firefox&version=71.0&version=72.0&version=72.0.1&version=72.0.2&date=%3E%3D2019-12-02T00%3A00%3A00.000Z&date=%3C2020-02-03T21%3A45%3A00.000Z&_facets=url&_facets=user_comments&_facets=install_time&_facets=version&_facets=address&_facets=moz_crash_reason&_facets=reason&_facets=build_id&_facets=platform_pretty_version&_facets=signature&_facets=useragent_locale&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

There wasn't any corruption in xul.dll itself (unfortunately), but two of the crashes had significant differences in ntdll. Probably some kind of "security" software. All the crashes that I looked at are also unique machines; eyeballing the crashes suggest that the pattern of unique machines continues for unexamined crashes as well.

So I don't think we can blame bad memory here.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

We're still seeing these crashes in the wild. Is there anything we can do to try to move this forward?

Comment 47

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #46)

We're still seeing these crashes in the wild. Is there anything we can do to try to move this forward?

I am waiting for more coverage of this code by TSan (suspected race). There is no other idea at the moment to try.

:decoder, do you have any updated how much tests we now run under TSan?

Comment 48

[Christian Holler (:decoder)]

We currently run Mochitest-plain and xpcshell tests under TSan. If you think other test suites might be able to find this, please let me know and I'll schedule them manually on try. It might also be worth looking at our suppression list at mozglue/build/TsanOptions.cpp to see if anything in there might actually be suppressing the race (each race entry matches to the entire stack).

If all of that fails, we could try crafting some fuzzing target that exercises the code in question and run it under TSan, but that's a long shot.

Comment 49

[Honza Bambas (:mayhemer)]

Studding more deeply this crash report.

• ESI should be this, at 1B0BCB30, which is unclear if it's right, but I would expect very likely to be unharmed.
• EAX is (at that point) keeping the value of the pointer as read from this->mSandboxedLoadingPrincipal.mRawPtr, which is at this+14h and which already contains a wrong value (00400000)
• We further dereference it and call Release of the instance (that+8), that is where the crash occurs because we call [ECX+8] while ECX is FFFFF7B0.

I wonder why the system (win x86) doesn't already crash while dereferring the memory at 00400000.

Studding [@ mozilla::net::nsHttpChannel::~nsHttpChannel] - Firefox 77.0.1 Crash Report - Report ID: 612d742c-95bd-418c-9eed-54ee50200701

• We are dereferencing nsHttpChannel::mCacheEntry
• RSI = 00000000286BA000, which seems reasonable
• RCX is filled with RSI+810h (this+offset of mCacheEntry) and is already wrong (0000270000000000)
• we are about to call Release of that object and derefercing this wrong pointer is crashing

So, this looks like the heap was damaged and/or overwritten. According the frequency and what I see in dumps, I more suspect a heap damage or OOB write, which will be very hard to find, yet more dangerous...

Comment 50

[Honza Bambas (:mayhemer)]

(In reply to Christian Holler (:decoder) from comment #48)

We currently run Mochitest-plain and xpcshell tests under TSan. If you think other test suites might be able to find this, please let me know and I'll schedule them manually on try.

This is hard to say. The crash is too rare even on the Release channel. Eventually we should be running everything with TSan. But mochitests should be a relatively good coverage for this already.

It might also be worth looking at our suppression list at mozglue/build/TsanOptions.cpp to see if anything in there might actually be suppressing the race (each race entry matches to the entire stack).

Nothing that would hit me.

If all of that fails, we could try crafting some fuzzing target that exercises the code in question and run it under TSan, but that's a long shot.

I wasn't even able to narrow down in what area of the code this could be, so hard to say how to build a fuzzing target :(

I'm running out of ideas and I think I'll stop hijacking this bug, dis-assigning for now.

Comment 51

[Jens Stutte [:jstutte]]

Are we sure we do not have two different crashes here?

The LoadInfo::Release variants that point here

The nsHttpChannel::Release variants that point here

At least I cannot see much common points in the stack traces other than the objects seem to be cycle collected.

Comment 52

[Jens Stutte [:jstutte]]

Oh, and the @ mozilla::net::CookieSettings::Release seems to happen only on 68 ESR, thus I assume we can ignore this now.

Comment 53

[Jens Stutte [:jstutte]]

FWIW, picking a random dump and cracking it up, I find:

00007FFA78B333A4 48 8B 8E D0 07 00 00 mov         rcx,qword ptr [rsi+7D0h]  (RSI = 000002C60879A000) 
00007FFA78B333AB 48 85 C9             test        rcx,rcx  (RCX = 0000000004000000)
00007FFA78B333AE 0F 85 20 05 00 00    jne         mozilla::net::nsHttpChannel::~nsHttpChannel+9F4h (07FFA78B338D4h)
...
00007FFA78B338D4 mov         rax,qword ptr [rcx] **** CRASH

qword ptr [rsi+7D0h] seems to be nsCOMPtr<nsIChannel> mRedirectChannel;.

IIUC this means, that we try to release an invalid smart pointer during NS_IMPL_RELEASE(HttpBaseChannel).

Edit: This is mostly the same as already discussed starting in comment 49.

Comment 54

[Jens Stutte [:jstutte]]

The current crashes seem to have a common denominator: They all happen during DeferredFinalize. My theory would then be the following:

• The cycle collector finds a cycle containing a nsHttpChannel instance and schedules a DeferredFinalizer for it. This happens passing and storing a raw pointer in the runnable, IIUC.
• In the meantime, someone else "finds a pointer" to this instance (probably stored somewhere else as raw pointer?) and somehow decreases its refcount, which goes to zero and releases the object synchronously
• Now the DeferredFinalizeris executed and acts on an already freed (and maybe even re-used memory) instance of nsHttpChannel, trying to release its members who contain invalid pointers.

Does this make sense, Kershaw?

Unfortunately we do not know, where step 2 could happen, though.

Comment 55

[Jens Stutte [:jstutte]]

Olli, you said you are interested to look at this.

Comment 56

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

Yeah, I went through quite a bit of code, but didn't yet find the issue.

DeferredFinalizer has a strong reference, it is just stored in a raw pointer, to the relevant objects, and it calls Release to delete the objects.
And cycle collector can't really find nsHttpChannel, since that isn't cycle collectable.
Comment 38 is still very valid.

Comment 57

[Kershaw Chang [:kershaw]]

(In reply to Jens Stutte [:jstutte] from comment #54)

The current crashes seem to have a common denominator: They all happen during DeferredFinalize. My theory would then be the following:

• The cycle collector finds a cycle containing a nsHttpChannel instance and schedules a DeferredFinalizer for it. This happens passing and storing a raw pointer in the runnable, IIUC.
• In the meantime, someone else "finds a pointer" to this instance (probably stored somewhere else as raw pointer?) and somehow decreases its refcount, which goes to zero and releases the object synchronously
• Now the DeferredFinalizeris executed and acts on an already freed (and maybe even re-used memory) instance of nsHttpChannel, trying to release its members who contain invalid pointers.

Does this make sense, Kershaw?

Unfortunately we do not know, where step 2 could happen, though.

Sorry for the late reply.
I think your theory makes sense, but I still can't find the root cause. I'll start with adding some diagnostic assertions in order to get more information.

Comment 58

[Kershaw Chang [:kershaw]]

Attachment: Bug 1544127 - Always release load info on main thread, r=#necko

Comment 59

[Kershaw Chang [:kershaw]]

remove the stalled keyword.

Comment 60

[Kershaw Chang [:kershaw]]

Comment on attachment 9244565 [details]
Bug 1544127 - Always release load info on main thread, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unknown. This is a speculative fix.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch should be able to applied to old branches cleanly.
  However, this is not a real fix. We might want to uplift until this patch is proved to be helpful.
• How likely is this patch to cause regressions; how much testing does it need?: Low. This patch only makes sure the loadinfo is always released on main thread.

Comment 61

[Tom Ritter [:tjr]]

Comment on attachment 9244565 [details]
Bug 1544127 - Always release load info on main thread, r=#necko

Approved to land

Comment 62

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Always release load info on main thread, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/211be0eac817b9d62220cfea9d4fc9c94991efe2
https://hg.mozilla.org/mozilla-central/rev/211be0eac817

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Looks like we're still seeing crashes on Beta95.

Comment 64

[Kershaw Chang [:kershaw]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #63)

Looks like we're still seeing crashes on Beta95.

Thanks. I am aware of this.

Comment 65

[Kershaw Chang [:kershaw]]

So, I think the problem is not about LoadInfo, it's about the members of LoadInfo.
There are some members in LoadInfo that could be released off main thread:

• mCSPEventListener
• mCookieJarSettings
• mPerformanceStorage

Since LoadInfo is always released on main thread, it's possible that Release() of above members are called on/off main thread at the same time. It could leads to the crash in this bug. It seems the only way to avoid this is forcing Release() to be called on main thread.

Comment 66

[Kershaw Chang [:kershaw]]

Since LoadInfo is always released on main thread, it's possible that Release() of above members are called on/off main thread at the same time. It could leads to the crash in this bug. It seems the only way to avoid this is forcing Release() to be called on main thread.

Correction: Release() being called on two different threads should be fine as long as the reference counter is an atomic.
I think what could cause crash is the case described in this blog post.

In short, I think the problem could be more related to mCSPEventListener and mPerformanceStorage, since these two objects can be both accessed on main thread and worker thread.

Comment 67

[Kershaw Chang [:kershaw]]

Attachment: Bug 1544127 - Release some members manually, r=#necko

Comment 68

[Kershaw Chang [:kershaw]]

Comment on attachment 9253302 [details]
Bug 1544127 - Release some members manually, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, since we assume the reason of this crash is racing happens on the data members of LoadInfo.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This is a diagnostic patch to prove our theory. We don't need to apply this to older branches.
• How likely is this patch to cause regressions; how much testing does it need?: Low. This patch should not cause any regression.

Comment 69

[Tom Ritter [:tjr]]

Comment on attachment 9253302 [details]
Bug 1544127 - Release some members manually, r=#necko

Approved to land

Comment 70

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Release some members manually, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/b5a39a664f8bce4570276891f15c696ba0caf504
https://hg.mozilla.org/mozilla-central/rev/b5a39a664f8b

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Given that we don't generally see this crashes until Beta/Release and we just started a new cycle this week, did you want to nominate this latest patch for uplift?

Comment 72

[Kershaw Chang [:kershaw]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #71)

Given that we don't generally see this crashes until Beta/Release and we just started a new cycle this week, did you want to nominate this latest patch for uplift?

sounds good. I'll request a uplift.

Comment 73

[Kershaw Chang [:kershaw]]

Comment on attachment 9253302 [details]
Bug 1544127 - Release some members manually, r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: No impact. This is a diagnostic patch.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch only sets some members to null in destructor. The risk is low.
• String changes made/needed: N/A

Comment 74

[Dianna Smith [:diannaS]]

Comment on attachment 9253302 [details]
Bug 1544127 - Release some members manually, r=#necko

Approved for 96.0b3

Comment 75

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36ea10248941

Comment 76

[Dianna Smith [:diannaS]]

Comment on attachment 9253302 [details]
Bug 1544127 - Release some members manually, r=#necko

It was approved and it landed in b3 but need to clear the flag so that it doesn't come up in tracking queries

Comment 77

[Kershaw Chang [:kershaw]]

Based on this search, it seems there was no crash in Firefox bete 96.
However, I've found 3 crash reports in LoadInfo::ReleaseMembers which suggest this is related to LoadInfo::mTriggeringPrincipal.

Comment 78

[Kershaw Chang [:kershaw]]

IIRC, nsIPrincipal should be only accessed on main thread, so I am still puzzled that principals might cause this crash.
Anyway, I'll upgrade the assertions here to MOZ_DIAGNOSTIC_ASSERT and see if doing this can reveal something.

Comment 79

[Kershaw Chang [:kershaw]]

Attachment: Bug 1544127 - Make sure nsIPrincipal is only accessed on main thread, r=#necko

Comment 80

[Kershaw Chang [:kershaw]]

Comment on attachment 9258633 [details]
Bug 1544127 - Make sure nsIPrincipal is only accessed on main thread, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unknown. This patch is just a diagnostic patch.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: We should be able to backport this patch cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Low. This patch only change MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT.

Comment 81

[Tom Ritter [:tjr]]

Comment on attachment 9258633 [details]
Bug 1544127 - Make sure nsIPrincipal is only accessed on main thread, r=#necko

Approved to land and request uplift if desired

Comment 82

[Tom Ritter [:tjr]]

Pinging Christoph to make him aware of this landing as he might be interested

Comment 83

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #82)

Pinging Christoph to make him aware of this landing as he might be interested

ACK - thank you!

Comment 84

[Kershaw Chang [:kershaw]]

Comment on attachment 9258633 [details]
Bug 1544127 - Make sure nsIPrincipal is only accessed on main thread, r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: No impact at all. This is just a diagnostic patch.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch only changes MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT.
• String changes made/needed: N/A

Comment 85

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Make sure nsIPrincipal is only accessed on main thread, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/7f7e85f6dcc25d7655b7d0eed5955d3f359ad8f0
https://hg.mozilla.org/mozilla-central/rev/7f7e85f6dcc2

Comment 86

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

This just landed, so no assertions yet, but I think it may be on the right track.
There are some other crashes in https://crash-stats.mozilla.org/search/?signature=~nsJSPrincipals::Release which indicates your hunch is correct, Kershaw.

Comment 87

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258633 [details]
Bug 1544127 - Make sure nsIPrincipal is only accessed on main thread, r=#necko

a=me for 97.0b6, but be advised that diagnostic asserts were recently changed to apply to EARLY_BETA_OR_EARLIER instead of Nightly/DevEdition, so this patch will effectively be disabled on Beta for 97.0b7+.

Comment 88

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258633 [details]
Bug 1544127 - Make sure nsIPrincipal is only accessed on main thread, r=#necko

Landed for 97.0b6. Removing the approval flag to get this off the needs-uplift radar.
https://hg.mozilla.org/releases/mozilla-beta/rev/b950672772cb

Comment 89

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

And... we have a winner:
https://crash-stats.mozilla.org/report/index/30fd0e55-8cc8-4d42-8b73-0a24f0220123

---	---	---
0	nsJSPrincipals::AddRef()	caps/nsJSPrincipals.cpp:30
1	nsCOMPtr_base::assign_with_AddRef(nsISupports*)	xpcom/base/nsCOMPtr.cpp:38
2	static nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&)	dom/security/nsContentSecurityManager.cpp:1224
3	mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*)	netwerk/protocol/http/nsHttpChannel.cpp:5754
4	NS_MaybeOpenChannelUsingAsyncOpen(nsIChannel*, nsIStreamListener*)	netwerk/base/nsNetUtil.cpp:2594
5	mozilla::net::WebSocketChannel::BeginOpenInternal()	netwerk/protocol/websocket/WebSocketChannel.cp
6	mozilla::net::WebSocketChannel::Notify(nsITimer*)	netwerk/protocol/websocket/WebSocketChannel.cp
7	nsTimerImpl::Fire(int)	xpcom/threads/nsTimerImpl.cpp:652
8	nsTimerEvent::Run()	xpcom/threads/TimerThread.cpp:265
9	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp:1189
10	mozilla::net::nsSocketTransportService::Run()	netwerk/base/nsSocketTransportService2.cpp:119
11	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp:1189

Crashing Thread (7), Name: Socket Thread

Comment 90

[Kershaw Chang [:kershaw]]

Thanks, Valentin. I'll file another bug for the crash in nsJSPrincipals::AddRef.

Comment 91

[Kershaw Chang [:kershaw]]

bug 1752100 is filed.

Comment 92

[Kershaw Chang [:kershaw]]

bug 1752100 was landed on 98, so we have to wait a bit to see if bug 1752100 fixes this one.

Comment 93

[Kershaw Chang [:kershaw]]

It looks like bug 1752100 helped a bit, since the recent crash reports show no crash on nsIPrincipal objects.
However, we still have some other crashes. As this crash report shows, the problematic object might be mCSPEventListener.

Comment 94

[Kershaw Chang [:kershaw]]

Attachment: Bug 1544127 - Check if mCSPEventListener is only accessed on main thread, r=#necko

mCSPEventListener should be main thread only, so add assertions to check.

Comment 95

[Kershaw Chang [:kershaw]]

It looks like in all these crashes, ~nsJARChannel is in the stack.
Note that jesup's patch could also help here.

Comment 96

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:kershaw, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 97

[Kershaw Chang [:kershaw]]

Note that jesup's patch could also help here.

Unfortunately, Jesup's patch doesn't help a lot. This crash still happens.
Apparently, this crash is caused by double-releasing loadinfo, which makes it really difficult to debug.
I'd remove myself and try get someone else to work on this.

Comment 98

[Dragana Damjanovic [:dragana]]

This crash has the part of the signature the same to this one. Therefore we have reasons to believe that the cause of this crash is in nsStandardURL.

I will add a patch that will split the release of some members of LoadInfo into separate runnables. The patch will be beta only patch and it may help us diagnose this bug better.

Comment 99

[Dragana Damjanovic [:dragana]]

Attachment: Split releasing members of LoadInfo into a couple of runnables

Comment 100

[Dragana Damjanovic [:dragana]]

Comment on attachment 9284317 [details]
Split releasing members of LoadInfo into a couple of runnables

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch does not expose what is happening. The patch is just a helper patch to better diagnose the problem.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This is just a diagnostic patch and we do not need to backport it.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely.
• Is Android affected?: Yes

Comment 101

[Tom Ritter [:tjr]]

Comment on attachment 9284317 [details]
Split releasing members of LoadInfo into a couple of runnables

Approved to land and request uplift

Comment 102

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Split releasing members of LoadInfo into a couple of runnables r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/a80ea1de5be7c82c23e191894162ea9d7b353b67
https://hg.mozilla.org/mozilla-central/rev/a80ea1de5be7

Comment 103

[Dragana Damjanovic [:dragana]]

I looked at some crashes and a lot of them have static mozilla::dom::DeferredFinalizerImpl<nsISupports>::DeferredFinalize(unsigned int, void*) on stack. Some have CycleCollectedJSRuntime on stack.

On beta most of the crashes with mozilla::net::nsHttpChannel::~nsHttpChannel signature crash in mozilla::net::SimpleChannelChild::Release().

This link is a query with "proto-signature" containing DeferredFinalizer. Some of these crashes are not in necko, but may not have the same cause.

Is it possible that we have a problem with DeferredFinalizer implementation? Nika, I am not sure if you are the right person, do you have an inside about DeferredFinalizer?

Comment 104

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

DeferredFinalize issues are perhaps something more for me or mccr8 or jonco.
DeferredFinalize is called on most of the objects which have had a JS wrapper. That is way to release the actual object at safe time.
So it is not at all surprising to see it on stack rather often. (For cycle collected objects one would probably see something about SnowWhiteKiller on stack).
The actual issue is rather likely elsewhere. Someone calling release too often so that DeferredFinalize has deleted objects in its data structures

Comment 105

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #104)

DeferredFinalize issues are perhaps something more for me or mccr8 or jonco.
DeferredFinalize is called on most of the objects which have had a JS wrapper. That is way to release the actual object at safe time.
So it is not at all surprising to see it on stack rather often. (For cycle collected objects one would probably see something about SnowWhiteKiller on stack).
The actual issue is rather likely elsewhere. Someone calling release too often so that DeferredFinalize has deleted objects in its data structures

Probably the object is added 2 times to the table (probably mDeferredFinalizerTable).
Can we add some diagnostic asserts to detect when an object is added the second time? I am looking for new ideas how to debug this issue.

Comment 106

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

But if it was added two times, it means there were two js wrappers for the object (one probably just GCed and then the next one created and then GCed), so also AddRef was called twice.
Recreating a wrapper is totally normal operation.

Comment 107

[Randell Jesup [:jesup] (needinfo me)]

Since this is supposedly a double-free of a loadinfo (and/or others), one possible attack would be to move to-be-freed loadinfo's to quarantine and set a flag in them, such that if someone tries to free them a second time we'll assert. (We'd have a fixed size or something to the array holding them, and once they age out release them). We can also release any sub-resources when they go into quarantine to reduce the memory hit.
The next question after we ensure it is such a double-free is to find out where the double-free is coming from. That may involve capturing stacks for each item in quarantine (and somehow making sure they're included in the crashdump, which may be tricky -- or ship the data up to us separately, instead of actually crashing the browser immediately).
NI-ing glandium in case he has ideas

Comment 108

[Dragana Damjanovic [:dragana]]

Have we implemented something similar that we could reuse? Do we have such a generic structure already? Probably not otherwise you would link to it.

Comment 109

[Dragana Damjanovic [:dragana]]

Attachment: Use ProxyReleaseRunnable to proxy releasing of member of LoadInfo.

Comment 110

[Dragana Damjanovic [:dragana]]

I have prepared an update to the diagnostic patch. We will wait for the patch to reach Beta and see if we have new insights into what is happening here.

Comment 111

[Dragana Damjanovic [:dragana]]

Comment on attachment 9297967 [details]
Use ProxyReleaseRunnable to proxy releasing of member of LoadInfo.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch does not increase the likelihood of the bug being exploited. It is diagnostic patch that try to narrow down where is the ptoblem.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This is only a diagnostic patch so an uplift will not be needed.
  The patch should also cleanly apply to older versions.
• How likely is this patch to cause regressions; how much testing does it need?: unlikely and it is a change that only affects early-beta and nightly.
• Is Android affected?: Yes

Comment 112

[Dragana Damjanovic [:dragana]]

I will put the bug into the necko priority-review queue for monitoring.

I will not have time to work on this bug further, so I am unassigning myself.

Comment 113

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

The patch which landed (comment 102) is adding rather high overhead because it floods small tasks to the main thread.
Sure, it is early beta or earlier, but makes certain performance work hard.

Comment 114

[Tom Ritter [:tjr]]

Comment on attachment 9297967 [details]
Use ProxyReleaseRunnable to proxy releasing of member of LoadInfo.

Approved to land

Comment 115

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #113)

The patch which landed (comment 102) is adding rather high overhead because it floods small tasks to the main thread.
Sure, it is early beta or earlier, but makes certain performance work hard.

Do you have suggestions/preferences?

Comment 116

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1544127: Remove diagnostics from LoadInfo r=#necko

Comment 117

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9300698 [details]
Bug 1544127: Remove diagnostics from LoadInfo r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Pretty hard, this is a diagnostic patch. It does make it clear we're looking for some sort of double-free. They'd have to find a way to trigger it (which is what we're trying to do). Also: there was already diagnostic code here, this just updates it.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: We don't intend to uplift this patch
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions, though may increase memory use slightly. Try is green. Also will mitigate the performance issue with the previous diagnostic code here had.
• Is Android affected?: Yes

Comment 118

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Olli Pettay [:smaug][[e-mail]](pto-ish Oct 31 - Nov 4) from comment #113)

The patch which landed (comment 102) is adding rather high overhead because it floods small tasks to the main thread.
Sure, it is early beta or earlier, but makes certain performance work hard.

I think my new diagnostic patch will address your concern

Comment 119

[Tom Ritter [:tjr]]

Comment on attachment 9300698 [details]
Bug 1544127: Remove diagnostics from LoadInfo r=#necko

Approved to land. It looks like there's no point to uplifting it.

Comment 120

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed https://hg.mozilla.org/integration/autoland/rev/aa3067b8562d21e4418774365c741cd71ce952f3

Backed out fo causing multiple failures: https://hg.mozilla.org/integration/autoland/rev/6020c747f007d662862f3cf5d6f20d0129326448

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=f45xsrPFR1KSqRH6oGLV8w.0&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=aa3067b8562d21e4418774365c741cd71ce952f3
Marionette failure log: https://treeherder.mozilla.org/logviewer?job_id=395200277&repo=autoland&lineNumber=136603
Wd failure log: https://treeherder.mozilla.org/logviewer?job_id=395200792&repo=autoland&lineNumber=3033

Comment 121

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This had landed:

Diagnostics r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/f90b376b2c98
https://hg.mozilla.org/mozilla-central/rev/f90b376b2c98

Comment 122

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing crashes (bug 1799278):
https://hg.mozilla.org/mozilla-central/rev/46a303d1bb44

Comment 123

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Michal Novotny [:michal] from comment #11)

(In reply to PTO until September 9th from comment #6)

I'm wondering if a racy release of LoadInfo may be happening off-main-thread (meaning it's usually on the main thread, but sometimes off main thread), and that is causing the main-thread refcount to be wrong?

LoadInfo doesn't have thread-safe refcounting. We would get an assertion if it would be addrefed or released on a different thread. But there might be edge cases which do not happen on the testing infrastructure. We should IMO make it thread-safe and see what it does with crash stats.

We had made it thread-safe, and it hadn't solved the problems. This now-backed-out diagnostic patch moved it back to non-thread-safe, and added diagnostic asserts.

So bug bug 1799278 shows LoadInfo is released off-main-thread (and off worker threads). NS_GetFinalChannelURI() calls channel->LoadInfo(). Is/Should this be safe off-main-thread/off-worker-threads? The accesses seem to all be from StyleThreads, and calling ShouldResistFingerprinting(). If privacy_resistFingerprinting is false, it just returns, so there would be no LoadInfo access from off-main-thread. I'm guessing there are no tests in CI that set privacy_resistFingerprinting and hit these styling spots...

If we need to access this OMT/off-worker, we likely need a Mutex to protect fields. The previous thread-safe state would be unsafe if SetResultPrincipalURI() was called from a different thread than a thread calling NS_GetFileChannelURI() (which calls GetResultPrincipalURI()). And very likely there are other races in this situation

Note also that ShouldResistFingerprinting() calls channel->LoadInfo() as well. Note that dist/include/nsIChannel.h has:

 JS_HAZ_CAN_RUN_SCRIPT NS_IMETHOD GetLoadInfo(nsILoadInfo **aLoadInfo) = 0;
...
  inline already_AddRefed<nsILoadInfo> LoadInfo()
  {
    nsCOMPtr<nsILoadInfo> result;
    mozilla::DebugOnly<nsresult> rv = GetLoadInfo(getter_AddRefs(result));

which makes me even more concerned about this.
@smaug any thoughts?
Could all the crashes be from resistFingerprinting?

This might also explain some intermittents: bug 1784390, bug 1780558, bug 1780143, bug 1780590 (all thread 11).

See also bug 1415488

Comment 124

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

Hmm, even if stylo threads were synchronous (blocking the main thread), if there are multiple of them and they access LoadInfo at the same time, refcount could go wrong. That could explain this.

Comment 125

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

So, what we tried in the past - making the refcount thread safe, and dispatching the release to the main thread didn't work. I assume it's because accessing some loadInfo members such as principals off main thread can break the refcount for those, and then lead to a crash in the LoadInfo destructor. We could additionally add thread diagnostic asserts to all the methods that use principal members.

Comment 126

[Emilio Cobos Álvarez [:emilio]]

Hmm, yeah, it seems https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/dom/base/nsContentUtils.cpp#2219-2225 isn't hit on automation probably because of the quick bail-out above, and seems it should be able to be called from multiple threads via PreferredColorScheme.

That's totally broken. Also, we want the PreferredColorScheme to be as fast as possible since it's used from color resolution. Can we make nsContentUtils::ShouldResistFingerprinting(Document*) fast and thread-safe instead of doing this complex / slow check?

Comment 127

[Tom Ritter [:tjr]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #126)

Can we make nsContentUtils::ShouldResistFingerprinting(Document*) fast and thread-safe instead of doing this complex / slow check?

As far as making it fast, we could cache the result - we don't support changing the RFP behavior while a page is open and having it take effect. (At least we don't intend to support it reliably, YMMV depending on the specific attribute you're looking at.) Requiring a document reload is fine from that point of view.

As far as thread-safe; I'm less knowledgeable about that. We could maybe pre-compute the value at some point during Document creation where we have all the information we need and we know we're on main thread? We do need to look at several aspects of the LoadInfo to know the RFP status.

If that sounds workable, I can make a patch for it...

Comment 128

[Tom Ritter [:tjr]]

Never mind, I just saw Bug 1799435

Comment 129

[Randell Jesup [:jesup] (needinfo me)]

With any luck, this bug will be resolved with bug 1799435. We'll monitor for crashes after that lands and uplifts.

Comment 130

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

I wonder what sorts of crash reports we should be even seeing here currently.

Comment 131

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Remove diagnostics from LoadInfo r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/10ce25b3dd8746423bed541d7b38055033cc8053
https://hg.mozilla.org/mozilla-central/rev/10ce25b3dd87

Comment 132

[Randell Jesup [:jesup] (needinfo me)]

This will need to go to release and run there for a few weeks or a release before we're sure it's fixed, given the normal volume. NI to myself to check later

Comment 133

[Greg Hess]

Patch landed in 109, no crashes have been observed we will monitor in release this week and close if no further crashes.

Comment 134

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

Bug 1801315 landed to 108 and bug 1799435 to 107.

Comment 135

[Randell Jesup [:jesup] (needinfo me)]

If we have no crashes in 109 in another week, we can close this.

Comment 136

[Ryan VanderMeulen [:RyanVM]]

To be clear, we'd be attributing that to the patch which landed in comment 131? If so, it would be helpful if we could get an ESR patch that rolls up the various changes that landed in this bug (which weren't uplifted along the way) and an approval request.

Comment 137

[Ryan VanderMeulen [:RyanVM]]

Looks like a80ea1de5be7+10ce25b3dd87 graft cleanly to ESR and the resulting folded-up diff looks like the below:

diff --git a/netwerk/base/LoadInfo.cpp b/netwerk/base/LoadInfo.cpp
--- a/netwerk/base/LoadInfo.cpp
+++ b/netwerk/base/LoadInfo.cpp
@@ -787,31 +787,17 @@ LoadInfo::Release() {
     return 0;
   }
 
   return count;
 }
 
 NS_IMPL_QUERY_INTERFACE(LoadInfo, nsILoadInfo)
 
-void LoadInfo::ReleaseMembers() {
-  mCSPEventListener = nullptr;
-  mCookieJarSettings = nullptr;
-  mPerformanceStorage = nullptr;
-  mLoadingPrincipal = nullptr;
-  mTriggeringPrincipal = nullptr;
-  mPrincipalToInherit = nullptr;
-  mTopLevelPrincipal = nullptr;
-  mResultPrincipalURI = nullptr;
-  mCspToInherit = nullptr;
-  mUnstrippedURI = nullptr;
-  mAncestorPrincipals.Clear();
-}
-
-LoadInfo::~LoadInfo() { ReleaseMembers(); }
+LoadInfo::~LoadInfo() { MOZ_RELEASE_ASSERT(NS_IsMainThread()); }
 
 already_AddRefed<nsILoadInfo> LoadInfo::Clone() const {
   RefPtr<LoadInfo> copy(new LoadInfo(*this));
   return copy.forget();
 }
 
 already_AddRefed<nsILoadInfo> LoadInfo::CloneWithNewSecFlags(
     nsSecurityFlags aSecurityFlags) const {
diff --git a/netwerk/base/LoadInfo.h b/netwerk/base/LoadInfo.h
--- a/netwerk/base/LoadInfo.h
+++ b/netwerk/base/LoadInfo.h
@@ -54,17 +54,17 @@ using RedirectHistoryArray = nsTArray<ns
 /**
  * Class that provides an nsILoadInfo implementation.
  */
 class LoadInfo final : public nsILoadInfo {
   template <typename T, typename... Args>
   friend already_AddRefed<T> mozilla::MakeAndAddRef(Args&&... aArgs);
 
  public:
-  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_ISUPPORTS
   NS_DECL_NSILOADINFO
 
   // Used for TYPE_DOCUMENT load.
   static already_AddRefed<LoadInfo> CreateForDocument(
       dom::CanonicalBrowsingContext* aBrowsingContext,
       nsIPrincipal* aTriggeringPrincipal,
       const OriginAttributes& aOriginAttributes, nsSecurityFlags aSecurityFlags,
       uint32_t aSandboxFlags);

Randall, can you confirm that this looks like what we'd want to land on ESR?

Comment 138

[Randell Jesup [:jesup] (needinfo me)]

yes

Comment 139

[Randell Jesup [:jesup] (needinfo me)]

3 109 crashes
One is an e5e5 crash on android, however that's releasing an HttpTransactionShell. It looks like an e5e5 address got somehow stored in a refptr -- but that's hard to imagine how that happens without crashing -- or something wiped our structure with e5e5's.
The other two are a generic crash with a single-bit address from apparently mAuthProvider and a crash with a single-bit address releasing RefPtr<nsIEarlyHintObserver>

These all seem like different bugs than this one (LoadInfo); I advise we file new bugs for those and close this one.

Comment 140

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1544127: Remove diagnostics from LoadInfo. r=valentin

Comment 141

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 142

[Ryan VanderMeulen [:RyanVM]]

Randell, can you please nominate the ESR roll-up patch for uplift when you get a chance? Thanks!

Comment 143

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9315201 [details]
Bug 1544127: Remove diagnostics from LoadInfo. r=valentin

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Cleanup patch now that the main crash culprit has been fixed. More consistency with the mainline branches.
• User impact if declined: None, this is just a cleanup
• Fix Landed on Version: 109
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Per Slack discussion with jesup, this patch is just removing diagnostics that were added.

Approved for 102.8esr so we can keep our various release branches more consistent with each other.

Comment 144

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/70c173b7a025