1498510 - Crash in arena_t::DallocSmall | BaseAllocator::free | je_free | mozilla::dom::WorkerCSPEventListener::Release

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-2ef1eaf2-e2a1-4230-b37c-10a7a0181011.
=============================================================

Top 10 frames of crashing thread:

0 mozglue.dll arena_t::DallocSmall memory/build/mozjemalloc.cpp:3450
1 mozglue.dll BaseAllocator::free memory/build/mozjemalloc.cpp:4264
2 mozglue.dll je_free memory/build/malloc_decls.h:40
3 xul.dll mozilla::dom::WorkerCSPEventListener::Release dom/workers/WorkerCSPEventListener.cpp:51
4 xul.dll mozilla::net::InterceptedHttpChannel::SetReleaseHandle netwerk/protocol/http/InterceptedHttpChannel.cpp:1036
5 xul.dll bool mozilla::dom::WorkerPrivate::EnsureCSPEventListener dom/workers/WorkerPrivate.cpp:3456
6 xul.dll ?WorkerRun@CompileScriptRunnable@?A0x30C6BC27@dom@mozilla@@EEAA_NPEAUJSContext@@PEAVWorkerPrivate@23@@Z$c9819905468e1c5f2eb55f63ab0ae971 dom/workers/WorkerPrivate.cpp:361
7 xul.dll mozilla::dom::WorkerRunnable::Run dom/workers/WorkerRunnable.cpp:380
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1161
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519

=============================================================

There are 7 crashes (from 7 installations) in 63.0b13 with buildid 20181008155858. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1493629.

[1] https://hg.mozilla.org/releases/mozilla-beta/rev?node=be85aa71510b

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Hey :baku, I am assigning this one to you since this is going to hit release soon. Worst case we have to pref it off. Could you please take a look?

Comment 2

[Andrea Marchesini [:baku]]

I've already started working on it. I hope to have a quick solution soon.

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Worst case we have to pref it off.

Pref what off?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: csp2.patch

This is not a small patch. Just because CSP objects are reused by nested workers, I need to have a different way to pass nsICSPEventListener from WorkerPrivate to the necko world and, from there, to the CSP violation code.

This code will be removed when CSP will be moved/changed to clientInfo.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9018805 [details] [diff] [review]
csp2.patch

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

As discussed on slack it probably makes sense to move the CSPEventListener into the client instead of the loadInfo which will make the patch way smaller. Additionally ClientInfo will be a better fit because it's propagated correctly through all the API layers (e.g. fetch). I haven't seen anything else that I wouldn't accept. Thanks for fixing!

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 9018805 [details] [diff] [review]
csp2.patch

Here the comment improved as discussed on slack:

+    * The object in charged to receive CSP violation events. It can be null.
+    * This attribute will be merged into the CSP object eventually.
+    * See bug 1500908.
+    */
+  attribute nsICSPEventListener cspEventListener;

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9018805 [details] [diff] [review]
csp2.patch

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

Baku, I would prefer if we would not touch the LoadInfo constructor and also not change the NS_NewChannel* helpers. Can't we just use the getter/setter on the loadinfo to propagate the listener where needed?

Comment 8

[Andrea Marchesini [:baku]]

Attachment: csp3.patch

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9019042 [details] [diff] [review]
csp3.patch

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

Thanks baku for incorporating my suggestion.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/29f0a8f38d05b8d0d7ad63aebe715590f9813e02
https://hg.mozilla.org/mozilla-central/rev/29f0a8f38d05

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Given that we only ever saw crash reports for this bug from Beta users, I think we should proceed with a Beta uplift request soonish?

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 9019042 [details] [diff] [review]
csp3.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1472927

User impact if declined: A crash can occur

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: race condition

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): This patch moves nsICSPEventListener out of CSP object. The patch is not trivial, but it's big. medium complexity seems the right choice.

String changes made/needed: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9019042 [details] [diff] [review]
csp3.patch

It's a bit scary, but it's also a crash we don't want in 64 and may want to keep on the 63 dot release radar too. Approved for 64.0b4 so we can get feedback ASAP on the impact it has.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3cf4f972200c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Looks like this patch was effective based on the results from 64.0b4 and 64.0b5. Crash volume on release is significant enough that I think we should get this nominated for release approval as a possible ride-along for the next dot release.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 9019042 [details] [diff] [review]
csp3.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1472927

User impact if declined: A crash can be triggered by nested workers

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Stable on beta

String changes made/needed: none

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9019042 [details] [diff] [review]
csp3.patch

Crash fix that was effective on beta, approved for 63.0.3, thanks.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch for m-r. And it looks like at least dom/base/nsJSTimeoutHandler.cpp also needs some changes along the same lines.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: m-r

Comment 21

[Pascal Chevrel:pascalc]

Aryx, could you land the updated patch to mozilla-release? Thanks

Comment 22

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

https://hg.mozilla.org/releases/mozilla-release/rev/1b78230f1e90aaec18928b98a3b555ccbfa25410

Comment 23

[:Gijs (he/him)]

Just ran into this code for something not really related. This hunk - https://hg.mozilla.org/mozilla-central/rev/29f0a8f38d05#l15.73 - looks wrong. CSPService::AsyncOnChannelRedirect now looks like:

  nsCOMPtr<nsILoadInfo> loadInfo = oldChannel->GetLoadInfo();

  nsCOMPtr<nsICSPEventListener> cspEventListener;
  rv = loadInfo->GetCspEventListener(getter_AddRefs(cspEventListener));
  NS_ENSURE_SUCCESS(rv, rv);

  // if no loadInfo on the channel, nothing for us to do
  if (!loadInfo) {
    return NS_OK;
  }

(The middle block was added in this bug.)

Either the nullcheck should be removed or it needs to move above the use of loadInfo to avoid crashes. Are there warnings we can enable as errors for this type of thing? Am I misreading something?

Comment 24

[Andrea Marchesini [:baku]]

I filed bug 1518948 to fix this issue.