Closed
Bug 1268721
Opened 8 years ago
Closed 8 years ago
Null deref crash in GetSurfaceData() called from PuppetWidget::SetCursor()
Categories
(Core :: Widget, defect, P1)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: crash)
Crash Data
Attachments
(6 files)
3.04 KB,
patch
|
jimm
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.19 KB,
patch
|
jimm
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
Details | Diff | Splinter Review | |
2.06 KB,
patch
|
Details | Diff | Splinter Review | |
3.06 KB,
patch
|
Details | Diff | Splinter Review | |
4.20 KB,
patch
|
Details | Diff | Splinter Review |
Maybe GetDataSurface() can return null sometimes?
Assignee | ||
Comment 1•8 years ago
|
||
I don't see a lot of these, but it could be an easy small win for e10s stability.
tracking-e10s:
--- → ?
Assignee | ||
Comment 2•8 years ago
|
||
I'll just fix this. I think just returning failure if GetDataSurface() returned null should do it.
Assignee: nobody → continuation
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06c2076b37cb I'll attach -w diffs, which are easier to read.
Attachment #8747833 -
Flags: review?(jmathies)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8747834 -
Flags: review?(jmathies)
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
Updated•8 years ago
|
Attachment #8747833 -
Flags: review?(jmathies) → review+
Updated•8 years ago
|
Attachment #8747834 -
Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1193bfa38fdd https://hg.mozilla.org/integration/mozilla-inbound/rev/b65089fc5dac
Updated•8 years ago
|
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1193bfa38fdd https://hg.mozilla.org/mozilla-central/rev/b65089fc5dac
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 11•8 years ago
|
||
Andrew, any interest in uplifting this to 48 or 47?
Flags: needinfo?(continuation)
Assignee | ||
Comment 12•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Attachment #8747834 -
Flags: approval-mozilla-beta?
Attachment #8747834 -
Flags: approval-mozilla-aurora?
status-firefox47:
--- → affected
status-firefox48:
--- → affected
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+
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/6087393c8970 https://hg.mozilla.org/releases/mozilla-aurora/rev/598c9eb576ab
Comment 15•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Comment 19•8 years ago
|
||
(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.
Description
•