1347984 - Crash in EnqueuePromiseReactionJob called from nsJARChannel::OnStopRequest

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-5b904ab8-0afe-4928-a86d-01beb2170314.
=============================================================

There are a few dozen of these crashes across various channels in the last week. The ones with this signature that I looked at were all coming from nsJARChannel::OnStopRequest, into AsyncScriptLoader::OnStreamComplete, into js::PromiseObject::reject.

Comment 1

[Darkspirit]

before bp-61d6ccf4-a326-43ca-adba-5e62b0170419 happened, I installed the webextension only xpi from https://github.com/gorhill/uBlock/releases/download/1.12.1/uBlock0.webext.xpi (uBlock Origin) in my second firefox profile. Then I started it via about:profiles and the tabs from the last session were automatically opened before the immediate crash. I think this is my first "@ EnqueuePromiseReactionJob" crash.

Comment 2

[[:philipp]]

this crash signature seems to have regressed in volume in firefox 54, which is now entering into the beta phase. the first recorded crash was in 54.0a1 build 20170224030232 and it continued somewhat regularly thereafter...

the crashes are happening cross-platform but it's mainly os x and linux systems that are affected by it. in very early crash data from 54.0b1 the signature is accounting for 4.4% of all crashes in the content process.

Comment 3

[Darkspirit]

(In reply to Darkspirit from comment #1)
> before bp-61d6ccf4-a326-43ca-adba-5e62b0170419 happened, I installed the
> webextension only xpi from
> https://github.com/gorhill/uBlock/releases/download/1.12.1/uBlock0.webext.
> xpi (uBlock Origin) in my second firefox profile. Then I started it via
> about:profiles and the tabs from the last session were automatically opened
> before the immediate crash. I think this is my first "@
> EnqueuePromiseReactionJob" crash.

When I set extensions.webextensions.remote back to false (the default), I don't had any crashes anymore. I reported bug 1357729 in this context, which is resolved as duplicate of bug 1353060.

Comment 4

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: Crash volume seems to be climbing, cf. comment 2.

Till, is there any other diagnostic information that we can add to help debug this?  I know that you have said that you don't see what's going on in bug 1346012, but clearly something is going wrong. :)

Comment 5

[Marcia Knous [:marcia]]

Makes sense to track this based on Comment 4.

Comment 6

[Andy McKay]

Is this planning on being uplifted, we think this might be the causes of in the beta webextensions cohort.

Comment 7

[:shell escalante]

Hi Jason, Is there someone that owns this bug?  It sounds like a P1 fix that would be considered for uplift to 54... without a clear owner.

Comment 8

[Naveed Ihsanullah [:naveed]]

Shu can you have someone take a look at this?

Comment 9

[Shu-yu Guo [:shu]]

Till is still the person with most expertise in the promises code, who's already needinfo'd. Darkspirit's comment 1 STR is worth a try, hopefully it still reproduces.

Comment 10

[Darkspirit]

(Sorry, can't reproduce it anymore.)

Comment 11

[Till Schneidereit [:till]]

Attachment: More release asserts

After staring at this code for another few hours, I still don't even have a hunch about what might be going on. Given that this happens on all platforms, it seems very likely that this is in fact an internal bug, not something caused by meddling by, say, AV software.

This patch adds a few additional release asserts that I have at least some vague hope of getting triggered instead of crashing as right now.

I would very much like to know what's special about address 0x4c201b4, which is involved in about 50% of all crashes ...

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8869708 [details] [diff] [review]
More release asserts

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

No crashes on Nightly in the past 7 days? Not enough users or is it possible it got fixed there?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e654020c64
Add some more release asserts to promise job handling code. r=jandem

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8e654020c64

Comment 15

[[:philipp]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> No crashes on Nightly in the past 7 days? Not enough users or is it possible
> it got fixed there?
there aren't that many of those crashes happening on nightly - should we try to get the patch to beta in order to get more data? (also if it's mainly a diagnostic patch, should we keep the bug open)?

Comment 16

[Till Schneidereit [:till]]

(In reply to [:philipp] from comment #15)
> (In reply to Jan de Mooij [:jandem] from comment #12)
> > No crashes on Nightly in the past 7 days? Not enough users or is it possible
> > it got fixed there?
> there aren't that many of those crashes happening on nightly - should we try
> to get the patch to beta in order to get more data? (also if it's mainly a
> diagnostic patch, should we keep the bug open)?

Yes, I intended for this to be uplifted. You're right on leaving the bug open, too.

Comment 17

[Till Schneidereit [:till]]

Comment on attachment 8869708 [details] [diff] [review]
More release asserts

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: no immediate impact, this is adding investigatory release asserts, which we want more users to exercise.
[Is this code covered by automated tests?]: yes, to the extent possible
[Has the fix been verified in Nightly?]: n/a
[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?]: Only adds release asserts that can only be triggered in situations where we'd otherwise be guaranteed to crash.
[String changes made/needed]: n/a

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8869708 [details] [diff] [review]
More release asserts

Diagnostic patch, let's uplift to beta for this Thursday's build.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/adb1f8ff7655

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Till Schneidereit [:till] from comment #17)
> [Is this code covered by automated tests?]: yes, to the extent possible
> [Has the fix been verified in Nightly?]: n/a
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Till's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> https://hg.mozilla.org/releases/mozilla-beta/rev/adb1f8ff7655

setting flags

Comment 22

[[:philipp]]

this is now showing up as [@ TriggerPromiseReactions ] in 54.0b11 and hitting the newly added MOZ_RELEASE_ASSERT(reactionVal.isObject())

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to [:philipp] from comment #22)
> this is now showing up as [@ TriggerPromiseReactions ] in 54.0b11 and
> hitting the newly added MOZ_RELEASE_ASSERT(reactionVal.isObject())

ni? to Till to re-look at this and see what the release assertions have uncovered.

Comment 24

[Calixte Denizet (:calixte)]

This signature is ranked #4 in content top-crash for beta. They all started in b11 (buildid 20170525155044).

Comment 25

[Julien Cristau [:jcristau] (back April 22)]

Gerry, this is a new crash in 54 with relatively high volume in beta (and the new release assert added in this bug is triggering on beta quite a lot).  Should we block on this?  Back out the new assertions?

Comment 26

[Gerry Chang [:gchang]]

Hi(In reply to Julien Cristau [:jcristau] from comment #25)
> Gerry, this is a new crash in 54 with relatively high volume in beta (and
> the new release assert added in this bug is triggering on beta quite a lot).
> Should we block on this?  Back out the new assertions?

Yes, I think we have to backout these new assertions.
NI sheriff to help on this.

Comment 27

[Carsten Book [:Tomcat]]

backed out from release in https://hg.mozilla.org/releases/mozilla-release/rev/94eea7315dcc25d85fda490636ac407362d2c5c3 and beta https://hg.mozilla.org/releases/mozilla-beta/rev/2125aa846c90398be9d3b6c01ff48de8265aeba3 for comment #24 and #25

Comment 28

[Nathan Froyd [:froydnj]]

(In reply to Gerry Chang [:gchang] from comment #26)
> Hi(In reply to Julien Cristau [:jcristau] from comment #25)
> > Gerry, this is a new crash in 54 with relatively high volume in beta (and
> > the new release assert added in this bug is triggering on beta quite a lot).
> > Should we block on this?  Back out the new assertions?
> 
> Yes, I think we have to backout these new assertions.
> NI sheriff to help on this.

Backing out the assertions doesn't solve anything, because we were already crashing before; those assertions were just added to see if we could get more information about the crash.  So we'll just get a different signature for the crashes without actually decreasing the crash volume, in which case Julien's other question--should we block on this?--becomes more relevant.

Comment 29

[Till Schneidereit [:till]]

(In reply to Gerry Chang [:gchang] from comment #26)
> Hi(In reply to Julien Cristau [:jcristau] from comment #25)
> > Gerry, this is a new crash in 54 with relatively high volume in beta (and
> > the new release assert added in this bug is triggering on beta quite a lot).
> > Should we block on this?  Back out the new assertions?
> 
> Yes, I think we have to backout these new assertions.
> NI sheriff to help on this.

I agree with Nathan, these asserts don't trigger any more crashes than we had before, they strictly move them to a slightly earlier point. Now that they have been backed out that's ok, too: I have all the information they could give already.

I've been looking at the crashes for quite a bit over the last couple of weeks, still to no avail. Hence I'm also not sure what blocking on them would achieve. I will continue to try to figure out what's going on.

Comment 30

[Gerry Chang [:gchang]]

Like :till mentioned in comment #29, there is no clue at this moment. We will leave this in 54.

Comment 31

[Nathan Froyd [:froydnj]]

Assuming this will be affecting 56.

Comment 32

[Marco Castelluccio [:marco]]

This is highly correlated with Avast on Release:
(85.71% in signature vs 00.40% overall) Addon "Avast SafePrice" = true
(85.71% in signature vs 00.99% overall) Addon "Avast Online Security" = true
(85.71% in signature vs 05.81% overall) Module "snxhk.dll" = true

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Still a huge volume crash, with 10K crashes in the last week, often but not always in the content process. 
Noting also that this crash inspires a lot of angry user comments.

Comment 34

[[:philipp]]

the MOZ_RELEASE_ASSERT(reactionVal.isObject()); is being hit on fennec numerous times in the last couple of days with the [@ ResolvePromise] signature.

this looks related to the ublock origin update to version 1.14.4 turning it into a webextension - the spike for this signature happened on the same day.

Correlations for FennecAndroid Release:
(99.92% in signature vs 00.71% overall) moz_crash_reason = MOZ_RELEASE_ASSERT(reactionVal.isObject())
(97.75% in signature vs 01.58% overall) Module "2029012401EexhtceanCspiuotnrSat.sqlite-shm" = true
(99.84% in signature vs 04.41% overall) Module "[e-mail]" = true
(99.92% in signature vs 06.45% overall) Addon "uBlock Origin" = true

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

This isn't showing up in huge volume on 56 or 57 but I assume it will when those move to release. 
I'm leaving a comment at https://github.com/gorhill/uBlock/issues/2795 to alert the uBlock developer(s).

Comment 36

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

Note that ([@ EnqueuePromiseReactionJob ]) is mostly windows read assertions (at least this week), and all either 0xfffffffffff or 0x4c201b4.  They *appear* to be on the line before the MOZ_RELEASE_ASSERT (which is being hit at lower frequency, and is also being hit on fennec).

The fixed address is odd...

Comment 37

[Marcia Knous [:marcia]]

#6 top overall browser crash in Fennec release.

Comment 38

[David Burns :automatedtester]

Just hit this for the first time in a newly updated Firefox 58. See https://crash-stats.mozilla.com/report/index/2462f4e0-ac5c-49f6-aad1-8a17e0170922

Comment 39

[Stuart Philp :sphilp]

I also just hit this on today's nightly OSX, first hard crash I've had in a while

https://crash-stats.mozilla.com/report/index/7e95bb09-46c3-4ffc-ad6b-cfb290170922

Comment 40

[Kan-Ru Chen [:kanru] (UTC+9)]

[Tracking Requested - why for this release]:

#1 top crash for Fennec nightly 20170925171818.

Correlations for FennecAndroid Nightly
(100.0% in signature vs 01.22% overall) moz_crash_reason = MOZ_RELEASE_ASSERT(reactionVal.isObject())
(100.0% in signature vs 30.29% overall) Addon "uBlock Origin" = true
(17.95% in signature vs 80.65% overall) build_id = 20170916100141
(86.32% in signature vs 27.68% overall) Addon "uBlock Origin" Version = 1.14.8

Comment 41

[Marcia Knous [:marcia]]

In early 57b3 this is also the top Mac browser crash. One comment: Just opened firefox and started typing in the adress bar. The keys I pressed didn’t start to show up.

Comment 42

[Till Schneidereit [:till]]

If someone has ideas for how to debug this further, I'd very much welcome those. Perhaps chaos mode in rr might stand a chance, don't know. As it is, I spent days trying to figure this out, tried to trace all code paths that could possibly cause the wrong value to be stored in this slot, and came up empty. I'm willing to spend more time on it, and that obviously would be important, but I definitely won't be able to do so until the end of next week as I'm traveling right now.

Comment 43

[Marcia Knous [:marcia]]

https://crash-stats.mozilla.com/signature/?signature=ResolvePromise is the highest volume signature on Android at the moment. Correlations for release, beta and nightly show a strong correlation to uBlock Origin" Version = 1.14.8. Perhaps trying to at least reproduce the crash on Android might help. ni on Ioana. I will try to get some URLs.

Comment 44

[Benjamin Bouvier [:bbouvier] (inactive)]

Just a random hint, maybe a red herring, but note the stacks in comment 38, comment 39, as well as all the 5 first stacks I've found on the link in comment 43 show mozilla::dom::GenerateRTCCertificateTask::Resolve() in the backtrace, which is generated via a webrtc generateCertificate call. Maybe it would be a nice place to add some log or look at the code?

https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/generateCertificate

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since it's a Fennec top crasher.

Comment 46

[alex_mayorga]

¡Hola Till!

Just crashed like this bp-c2fe8d82-9577-4888-95d2-3ea130170930

Is that crash this bug?

¡Gracias!
Alex

Comment 47

[André Bargull [:anba]]

It's possible to trigger that assertion because JS::ResolvePromise doesn't guard against reentrant calls (bug 1403988), but the test case for that scenario looks a bit contrived to me:

---
var CreatePendingPromise = getSelfHostedValue("CreatePendingPromise");
var p = CreatePendingPromise();
resolvePromise(p, {
    get then() {
        resolvePromise(p, [undefined]);
    }
});
---

Comment 48

[Nicholas Nethercote [inactive]]

This is the #4 Windows top crash in Nightly 20170929100122.

Comment 49

[Naveed Ihsanullah [:naveed]]

Can this functionality be selectively disabled or put behind a pref while we work on understanding the cause?

Comment 50

[Jan de Mooij [:jandem]]

Attachment: Patch

Crash dumps show the |reactions| object in TriggerPromiseReactions is a DeadObjectProxy. We can and should just pass this to EnqueuePromiseReactionJob - it knows how to handle this - but instead we fell through to the array case and there things go horribly wrong.

The patch also adds a nukeAllCCWs function to the JS shell. That makes it possible to write a shell test for this, and this function seems useful to have for fuzzing.

Comment 51

[Till Schneidereit [:till]]

Comment on attachment 8916035 [details] [diff] [review]
Patch

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

\o/, this is fantastic! I'd planned on spending the coming weekend on this, but I'm obviously very glad to see this fixed!

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/754a3e12321c
Check for dead object proxies in TriggerPromiseReactions. r=till

Comment 53

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

https://hg.mozilla.org/mozilla-central/rev/754a3e12321c

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and Release approval. I think we should strongly consider taking this patch if/when we ship a 56.0.2 release.

Comment 55

[Till Schneidereit [:till]]

Comment on attachment 8916035 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1313049
[User impact if declined]: Significantly higher crash rate
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[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, adds a type check that wasn't there before, nothing else.
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: none

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916035 [details] [diff] [review]
Patch

Fixes a top crasher, Beta57+

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9e89e842081

Comment 58

[Liz Henry (:lizzard) (relman/hg->git project)]

Here, I think the main effect would be on bringing down the crashes for Fennec on 56. This could be a good ridealong, maybe even a dot release driver for Fennec.

Comment 59

[Liz Henry (:lizzard) (relman/hg->git project)]

This is the #8 top crash for Fennec and we have a fix.  There isn't anything else as a mobile dot release driver for 56 though. 

Do we still think this is related to a specific version of ublock origin? 

Jandem or Till, want to request uplift to m-r?

Marcia, do you think this is worth a 56 dot release?

Comment 60

[Marcia Knous [:marcia]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #59)
> This is the #8 top crash for Fennec and we have a fix.  There isn't anything
> else as a mobile dot release driver for 56 though. 
> 
> Do we still think this is related to a specific version of ublock origin? 
> 
> Jandem or Till, want to request uplift to m-r?
> 
> Marcia, do you think this is worth a 56 dot release?

If we had another ride along maybe, but as discussed in IRC, on it own probably not worth it.

Comment 61

[Liz Henry (:lizzard) (relman/hg->git project)]

So far I don't see any other Fennec dot release driver. Maybe we should hold off.