Closed Bug 1265035 Opened 4 years ago Closed 4 years ago

Null deref crash in ~WorkerJSRuntime()

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: njn)

Details

(Keywords: crash, Whiteboard: btpp-active)

Crash Data

Attachments

(1 file, 1 obsolete file)

I see 19 of these in the last week, for the content process.

The crash is on this line:
  delete static_cast<WorkerThreadRuntimePrivate*>(JS_GetRuntimePrivate(rt));

Maybe rt is null? I could see that happening if Initialize() failed, though Crash Stats says this signature is older than bug 1247122.
~WorkerJSRuntime() certainly looks like it won't handle a failed Initialize() well. I'd start by fixing that.
I have an idea for handling this nicely.
Assignee: nobody → n.nethercote
Whiteboard: btpp-active
Actually, I'll just do the simple and obvious thing for now, and save my nice
idea for a follow-up bug.
Attachment #8742588 - Flags: review?(continuation)
Comment on attachment 8742588 [details] [diff] [review]
Make ~WorkerJSRuntime() handle Initialize() failure better

This looks reasonable to me, but all I know about worker shutdown is that it scares me.
Attachment #8742588 - Flags: review?(continuation) → review?(khuey)
Ho ho, CycleCollectedJSRuntime::Runtime() asserts that mJSRuntime is non-null,
so it's not appropriate to call it to determine if initialization succeeded. So
I've added a non-asserting MaybeRuntime() for this purpose.

All the more reason for my follow-ups...
Attachment #8742618 - Flags: review?(khuey)
Attachment #8742588 - Attachment is obsolete: true
Attachment #8742588 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/242336dca06d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
the signature seems to be quite common on 47 beta builds where it makes up ~0.5% of crashes. do you think it can be uplifted in terms of scope & risk?
Comment on attachment 8742618 [details] [diff] [review]
Make ~WorkerJSRuntime() handle Initialize() failure better

Approval Request Comment
[Feature/regressing bug #]: bug 1247122, maybe
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: this code runs frequently
[Risks and why]: low risk, it is just a null check
[String/UUID change made/needed]: none
Attachment #8742618 - Flags: approval-mozilla-beta?
Comment on attachment 8742618 [details] [diff] [review]
Make ~WorkerJSRuntime() handle Initialize() failure better

Crash fix, Beta47+
Attachment #8742618 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.