Closed
Bug 1265035
Opened 8 years ago
Closed 8 years ago
Null deref crash in ~WorkerJSRuntime()
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: mccr8, Assigned: n.nethercote)
Details
(Keywords: crash, Whiteboard: btpp-active)
Crash Data
Attachments
(1 file, 1 obsolete file)
3.19 KB,
patch
|
khuey
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
~WorkerJSRuntime() certainly looks like it won't handle a failed Initialize() well. I'd start by fixing that.
Assignee | ||
Comment 2•8 years ago
|
||
I have an idea for handling this nicely.
Assignee: nobody → n.nethercote
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8742588 -
Attachment is obsolete: true
Attachment #8742588 -
Flags: review?(khuey)
Attachment #8742618 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/242336dca06d789d466a2ac7700405c02e7ba487 Bug 1265035 - Make ~WorkerJSRuntime() handle Initialize() failure better. r=khuey.
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/242336dca06d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 8•8 years ago
|
||
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?
status-firefox47:
--- → affected
Reporter | ||
Comment 9•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/53445c568c7c
You need to log in
before you can comment on or make changes to this bug.
Description
•