Null deref crash in GetSurfaceData() called from PuppetWidget::SetCursor()

RESOLVED FIXED in Firefox 47

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({crash})

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(crash signature)

Attachments

(6 attachments)

Maybe GetDataSurface() can return null sometimes?
Keywords: crash
I don't see a lot of these, but it could be an easy small win for e10s stability.
tracking-e10s: --- → ?
I'll just fix this. I think just returning failure if GetDataSurface() returned null should do it.
Assignee: nobody → continuation
SourceSurfaceD2D1::GetDataSurface() can return nullptr, which means nsContentUtils::GetSurfaceData() can be passed a null pointer for the first argument. So either it needs to handle a null pointer, or the call sites should null check.
For those who think prevent is better than cure, I just uploaded a patch in bug 1266651 that fixes this in a way that makes the fact that nsContentUtils::GetSurfaceData()'s first argument can be null. It's not in a landable state, but is a good proof-of-concept.
Attachment #8747833 - Flags: review?(jmathies) → review+
Attachment #8747834 - Flags: review?(jmathies) → review+
Blocks: e10s-crashes
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/1193bfa38fdd
https://hg.mozilla.org/mozilla-central/rev/b65089fc5dac
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Andrew, any interest in uplifting this to 48 or 47?
Flags: needinfo?(continuation)
Comment on attachment 8747833 [details] [diff] [review]
part 1 - Use early continue in TransferableToIPCTransferable.

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: some crashes, though this is only the #135 content process crash on beta.
[Describe test coverage new/current, TreeHerder]: I think there's some tests for this code.
[Risks and why]: Low. These patches just add null checks, and this is e10s-only code.
[String/UUID change made/needed]: none
Flags: needinfo?(continuation)
Attachment #8747833 - Flags: approval-mozilla-beta?
Attachment #8747833 - Flags: approval-mozilla-aurora?
Attachment #8747834 - Flags: approval-mozilla-beta?
Attachment #8747834 - Flags: approval-mozilla-aurora?
Comment on attachment 8747833 [details] [diff] [review]
part 1 - Use early continue in TransferableToIPCTransferable.

Crash fix, Aurora48+, Beta47+
Attachment #8747833 - Flags: approval-mozilla-beta?
Attachment #8747833 - Flags: approval-mozilla-beta+
Attachment #8747833 - Flags: approval-mozilla-aurora?
Attachment #8747833 - Flags: approval-mozilla-aurora+
Attachment #8747834 - Flags: approval-mozilla-beta?
Attachment #8747834 - Flags: approval-mozilla-beta+
Attachment #8747834 - Flags: approval-mozilla-aurora?
Attachment #8747834 - Flags: approval-mozilla-aurora+
has problems uplifting to beta:

grafting 344341:6087393c8970 "Bug 1268721 - Part 1 - Use early continue in TransferableToIPCTransferable. r=jimm, a=ritu"
merging dom/base/nsContentUtils.cpp
warning: conflicts while merging dom/base/nsContentUtils.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(continuation)
I rebased these for beta. Thanks!
Flags: needinfo?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #18)
> I rebased these for beta. Thanks!

cool, thanks - landed !

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/ad37649f5860
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/f4db6cf4303f
You need to log in before you can comment on or make changes to this bug.