1490009 - Crash in OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::gc::StoreBuffer::putCell

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-19001319-8a54-49f5-a5a3-dc1990180910.
=============================================================

New Windows content crash showing up in the 63 betas: https://bit.ly/2x2DahT. No comments and the only URL that shows up is facebook. Doesn't appear to affect 62 and no crashes so far in 64 nightly.

Top 10 frames of crashing thread:

0 xul.dll js::AutoEnterOOMUnsafeRegion::crash js/src/vm/JSContext.cpp:1628
1 xul.dll js::gc::StoreBuffer::putCell js/src/gc/StoreBuffer.h:461
2 xul.dll JS::HeapObjectPostBarrier js/src/gc/Barrier.h:269
3 xul.dll static bool mozilla::CycleCollectedJSContext::EnqueuePromiseJobCallback xpcom/base/CycleCollectedJSContext.cpp:272
4 xul.dll JSRuntime::enqueuePromiseJob js/src/vm/Runtime.cpp:653
5 xul.dll static bool EnqueuePromiseReactionJob js/src/builtin/Promise.cpp:1008
6 xul.dll static bool ResolvePromise js/src/builtin/Promise.cpp:1052
7 xul.dll static bool FulfillMaybeWrappedPromise js/src/builtin/Promise.cpp:1086
8 xul.dll static bool ResolvePromiseInternal js/src/builtin/Promise.cpp:823
9 xul.dll js::PromiseObject::resolve js/src/builtin/Promise.cpp:4154

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

Comment 1

[Marcia Knous [:marcia]]

Adding another signature seen in 63 beta. 63 beta 4 has over 100 crashes so far.

Comment 2

[Marcia Knous [:marcia]]

Hello Steven - this is a new JS crash which is emerging in 63 - can someone triage this bug? Thanks in advance.

Comment 3

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 63 as the volume on beta is sizable. Steven could you triage this bug and help find an owner please? Thanks

Comment 4

[Steven DeTar [:sdetar]]

Jason: during your triage of the JS engine bugs this week, could we make sure this bug is triaged.  Thank you

Comment 5

[Jason Orendorff [:jorendorff]]

P1 to investigate, at least

Comment 6

[Jon Coppeard (:jonco)]

I don't know why this just started happening, but here's what I think is going on:

Whenever a promise is resolved or rejected the JS engine tells the embedding to create a job to handle it (by calling back into the engine) at the appropriate time.  These job objects frequently contain pointers to JSObject in the nursery, and so we have to record them in our storebuffer.  If a script uses a lot of promises then this will result in there being many store buffer entries.

(It occurs to me that this may have started because we're sweeping the nursery less frequently now.)

These job objects are cycle collected and so may persist until the next CC even after the job has been run and the object is no longer used.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1490009-reset-promise-callback

Here's a patch I think will help.  This clears all the JSObject references in the callback object after use.  This will remove the storebuffer entries ASAP rather than waiting for the next minor GC.

It also occurs to me that this could be a more general problem with CC'ed things holding GC things live longer than necessary.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 9010582 [details] [diff] [review]
bug1490009-reset-promise-callback

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

::: xpcom/base/CycleCollectedJSContext.cpp
@@ +226,5 @@
>      if (global && !global->IsDying()) {
>        mCallback->Call("promise callback");
>        aAso.CheckForInterrupt();
>      }
> +    mCallback->Reset();

Please add a comment here about why you are doing this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c173cb530f63
Clear CallbackObject fields after use for promise job to avoid tenuring objects unnecessarily r=mccr8

Comment 10

[Jon Coppeard (:jonco)]

I'd like to leave this open until we see whether it fixes the problem.

Comment 11

[Andreea Pavel [:apavel]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

There are explicit guarantees that CallbackObject provides about when CallbackOrNull() can return null, and the newly-added API totally violates those guarantees.  Worse yet, it didn't even adjust the corresponding documentation, so consumers would think that the guarantees are still being provided.

As a bare minimum, the new API needs to come with documentation explaining what invariants it's violating, and the existing documentation about the invariants needs to be updated accordingly.

Better yet, in addition to the documentation the API would be private with a friend declaration to allow this one callsite.

Even better would be to rejigger the whole thing to have a concept of "run-once" callback, which auto-disappears  after you Call() it.  Exactly how to set that up is not 100% clear to me yet, but conceptually it would be the best solution.

Comment 13

[Andrew McCreight [:mccr8]]

Yeah, I guess I should have checked more about whether it was okay or not to just randomly clear. I didn't realize there was this particular invariant. This should be backed out then. It'll still have been in a Nightly, so we can see if it helps the original issue, maybe.

I don't entirely understand how this violates the description on CallbackOrNull(). Doesn't JS run in the mCallback->Call()? Is the problem that it is expected that CallbackOrNull() can return null only when certain particular kinds of JS are run (as specified in the second sentence of the comment) or that sometimes we don't actually run mCallback->Call()?

Comment 14

[Boris Zbarsky [:bzbarsky]]

The invariant being violated is the second sentence:

  // It will only return null
  // after a JavaScript caller has called nukeSandbox on a Sandbox object, and
  // the cycle collector has had a chance to run.

That can't happen without script running, but not all script running can cause that.

Comment 15

[Andrew McCreight [:mccr8]]

It looks like Jon is on PTO. Sheriffs, please back this patch out.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/mozilla-central/rev/99723bad4d198575d4888af2cc103fa325719e5e

Comment 17

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

could this be related to bug 1488164 which has the same spike time?  (of course, it could just be two bugs that uplifted and caused OOMs in beta) (note: I think jonco is on PTO per above)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Doesn't seem related to me.

Comment 19

[Jon Coppeard (:jonco)]

Attachment: bug1490009-reset-promise-callback v2

Sorry about that.  How does this version look?  I updated the doc comments and make Reset() private.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9013311 [details] [diff] [review]
bug1490009-reset-promise-callback v2

>+  // this point. This should only be called if the object is know not to be used

"known"

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cae1a7f33840
Clear CallbackObject fields after use for promise job to avoid tenuring objects unnecessarily r=bz

Comment 22

[Raul Gurzau (:RaulG)]

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

Comment 23

[Ionuț Goldan [:igoldan]]

We gained some perf boosts!

== Change summary for alert #16377 (as of Tue, 02 Oct 2018 09:21:35 GMT) ==

Improvements:

 16%  tp5o_webext responsiveness linux64 pgo e10s stylo         1.23 -> 1.03
 16%  tp5o_webext responsiveness windows7-32 opt e10s stylo     1.20 -> 1.01
 15%  tp5o_webext responsiveness linux64 opt e10s stylo         1.39 -> 1.18
 12%  tp5o_webext responsiveness linux64-qr opt e10s stylo      1.96 -> 1.73

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16377

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Jon, can we uplift this patch to beta as almost all the  crashes affect 63?

Comment 25

[Jon Coppeard (:jonco)]

Comment on attachment 9013311 [details] [diff] [review]
bug1490009-reset-promise-callback v2

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Hopefully this will reduce the volume of OOM crashes on beta.

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: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a simple change and problems would have resulted in immediate test failures on central.

String changes made/needed: None.

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9013311 [details] [diff] [review]
bug1490009-reset-promise-callback v2

OOM Crash fix, uplift approved for 63 beta 12, thanks.

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01dd14f85e40

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

I see a perf win here:
== Change summary for alert #16386 (as of Tue, 02 Oct 2018 18:40:23 GMT) ==

Improvements:

 14%  tp5o_webext responsiveness windows7-32 opt e10s stylo     1.20 -> 1.03

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16386

Comment 29

[[:philipp]]

the volume of [@ OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::gc::StoreBuffer::putCell ] crashes doesn't look like it has significantly reduced on beta since the patch landed in 63.0b12.

Comment 30

[Jon Coppeard (:jonco)]

I don't know what's happening here.  Note that this is an OOM crash so the cause may not be related to this signature at all.

It's hard to know for sure but low volume of crashes like this seem to show up in nightly in the 20180823220048 build.

Comment 31

[Jon Coppeard (:jonco)]

Unassigning myself as I'm not actively working on this.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Steven, this is currently tracking64+ and unassigned. With only a couple weeks left in this cycle, is this on the radar still?

Comment 33

[Steven DeTar [:sdetar]]

Jon, can you help answer Ryan's question?  Is there anything we could be doing here?  It seems this is not on our radar at this point, is that accurate?

Comment 34

[Jon Coppeard (:jonco)]

I don't think there's anything we can do here.  We crashing because we're running out of memory so something has made us allocate more, but it's not necessarily the code in the stack trace.

Comment 35

[Julien Cristau [:jcristau]]

Too late to do anything for 64.

Comment 36

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

Marking this fix-optional for 65 and 66 to remove it from repeated triage.

Comment 37

[Nicolas B. Pierron [:nbp]]

Based on comment 30 and comment 34, this bug seems to be non-actionable.

Comment 38

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

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 39

[Andrei Purice]

Marking this as Resolved > Worksforme since there are no more crashes with this signature in the past 6 months.

Comment 40

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

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.