1344527 - Crash in mozilla::dom::ToJSValue called from ReadScriptAsync

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-0df28f26-a0c4-4be6-8f9f-f94fd2170205.
=============================================================

ToJSValue() dom/bindings/ToJSValue.cpp:74
ReadScriptAsync() js/xpconnect/loader/mozJSSubScriptLoader.cpp:423
MutatePrep() xpcom/string/nsTSubstring.cpp:157
DoLoadSubScriptWithOptions() js/xpconnect/loader/mozJSSubScriptLoader.cpp:684
LoadSubScriptWithOptions() js/xpconnect/loader/mozJSSubScriptLoader.cpp:558

Null-deref (offset 0x10).  Low-frequency crasher, appears to be a regression in 52 (all older crashes (few) in ToJSValue() have different stacks.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I saw :jandem's footprints on ReadScriptAsync() landed in 51 (bug 1292892), perhaps he has ideas of what's up here.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I saw :jandem's footprints on ReadScriptAsync() landed in 51 (bug 1292892),
> perhaps he has ideas of what's up here.

oh, but this one seems appear since 52, hmmm...

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > I saw :jandem's footprints on ReadScriptAsync() landed in 51 (bug 1292892),
> > perhaps he has ideas of what's up here.
> 
> oh, but this one seems appear since 52, hmmm...

Not really meant to clear the NI flag :) Let's see if :jandem thinks of anything related here, thanks!

Comment 4

[Jan de Mooij [:jandem]]

Bug 1292892 is unrelated. The code in this function [0] does look a bit suspicious, though:

    RefPtr<Promise> promise = Promise::Create(globalObject, result);
    if (result.Failed()) {
      promise = nullptr;
    }

    DebugOnly<bool> asJS = ToJSValue(jsapi.cx(), promise, retval);

Then in ToJSValue [1] we do:

  aValue.setObject(*aArgument.PromiseObj());

How is that okay if |promise| can be nullptr?

Till, any thoughts?

[0] http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/js/xpconnect/loader/mozJSSubScriptLoader.cpp#416-421

[1] http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/dom/bindings/ToJSValue.cpp#71

Comment 5

[Jan de Mooij [:jandem]]

Actually let's NI fabrice as suggested by blame data.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That code is totally not OK.  ToJSValue with expects non-null pointers held in smartptr types it's passed.

I have no idea why this would be any different in 52 unless we're having Promise::Create fail more often here or something.  

This code _also_ leaves an unhandled exception on "result" if the Create() fails.

I can't tell whether the intended behavior on promise creation failure here is "ignore it, return null to the caller, and press on" or "throw an exception to the caller".  The latter should generally make more sense, because the former gives the caller no way to know when the read is done.

Of course the "press on" codepath is broken too; it will null-deref crash in AutoRejectPromise::ResolvePromise if the load succeeds.

Needinfo for the reviewers of this code in terms of how it should behave.

Comment 7

[[:fabrice] Fabrice Desré]

Nothing to add to what bz wrote.

Comment 8

[Andrew McCreight [:mccr8]]

I reviewed the CC parts. I don't know anything about promises or the script loader. I can try to put something together if nobody else wants to.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Anyway, if we want the throwing behavior, we just replace "promise = nullptr;" with "return result.StealNSResult();".

If we want the pressing on behavior, probably need to audit a bit and add some null-checks.

In either case, it's worrying that this is happening at all.  :(

Comment 10

[Andrew McCreight [:mccr8]]

I see about 8 crashes right now in the last week. About half of them have system memory use of 60% to 93%, so it could just be an OOM. The other half are from a single installation on some localhost URL so something weird might just be going on with one user's system.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> Anyway, if we want the throwing behavior, we just replace "promise =
> nullptr;" with "return result.StealNSResult();".

This is definitely what we should do. I must have missed this during review, because there were several other things around that code that I was commenting on:

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1150106&attachment=8595065

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

Review commit: https://reviewboard.mozilla.org/r/118682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118682/

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

https://reviewboard.mozilla.org/r/118682/#review120638

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1b8f1aa1d6f
Give up in ReadScriptAsync if we can't create a promise. r=bholley

Comment 15

[Andrew McCreight [:mccr8]]

The volume of the crash does not justify backporting to 52.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

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

Please consider nominating this for 54 and 53 when you feel comfortable doing so

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1150106
[User impact if declined]: rare crashes
[Is this code covered by automated tests?]: not this particular error path
[Has the fix been verified in Nightly?]: not really reproducible
[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?]: probably not
[Why is the change risky/not risky?]: we only rarely hit these conditions at all, and it just makes it return early, so hopefully not much goes wrong
[String changes made/needed]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

As discussed with mccr8 on IRC, I'm not asking for an immediate uplift to esr52 but I want to keep it on the radar for consideration down the road since it's more broadly-scoped release than our past ESRs have been.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

Fix a crash. Beta53+ and Aurora54+.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37345f47bb30

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

52.0esr is showing up a lot in crash-stats. Let's take this there too.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

crash fix for esr52

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d4612b14c907

Comment 26

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Andrew's assessment on manual testing needs (see Comment 18).