Closed Bug 1344527 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPConnect, defect, major)

48 Branch
x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jesup, Assigned: mccr8)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
I saw :jandem's footprints on ReadScriptAsync() landed in 51 (bug 1292892), perhaps he has ideas of what's up here.
Flags: needinfo?(jdemooij)
(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...
Flags: needinfo?(jdemooij)
(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!
Flags: needinfo?(jdemooij)
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
Flags: needinfo?(jdemooij) → needinfo?(till)
Actually let's NI fabrice as suggested by blame data.
Flags: needinfo?(till) → needinfo?(fabrice)
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.
Flags: needinfo?(continuation)
Flags: needinfo?(bobbyholley)
Nothing to add to what bz wrote.
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(continuation)
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.  :(
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.
Assignee: nobody → continuation
(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
Flags: needinfo?(bobbyholley)
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
Attachment #8845562 - Flags: review?(bobbyholley) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1b8f1aa1d6f
Give up in ReadScriptAsync if we can't create a promise. r=bholley
The volume of the crash does not justify backporting to 52.
https://hg.mozilla.org/mozilla-central/rev/a1b8f1aa1d6f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please consider nominating this for 54 and 53 when you feel comfortable doing so
Flags: needinfo?(continuation)
Blocks: 1150106
Flags: needinfo?(continuation)
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
Attachment #8845562 - Flags: approval-mozilla-beta?
Attachment #8845562 - Flags: approval-mozilla-aurora?
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 on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

Fix a crash. Beta53+ and Aurora54+.
Attachment #8845562 - Flags: approval-mozilla-beta?
Attachment #8845562 - Flags: approval-mozilla-beta+
Attachment #8845562 - Flags: approval-mozilla-aurora?
Attachment #8845562 - Flags: approval-mozilla-aurora+
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.
Attachment #8845562 - Flags: approval-mozilla-esr52?
Comment on attachment 8845562 [details]
Bug 1344527 - Give up in ReadScriptAsync if we can't create a promise.

crash fix for esr52
Attachment #8845562 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Setting qe-verify- based on Andrew's assessment on manual testing needs (see Comment 18).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.