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

RESOLVED FIXED in Firefox 47

Status

()

Core
Widget
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({crash})

Trunk
mozilla49
crash
Points:
---

Firefox Tracking Flags

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

Details

(crash signature)

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
Maybe GetDataSurface() can return null sometimes?
(Assignee)

Updated

2 years ago
Keywords: crash
(Assignee)

Comment 1

2 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

2 years ago
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8747833 [details] [diff] [review]
part 1 - Use early continue in TransferableToIPCTransferable.

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

2 years ago
Created attachment 8747834 [details] [diff] [review]
part 2 - Null check first argument to nsContentUtils::GetSurfaceData().
Attachment #8747834 - Flags: review?(jmathies)
(Assignee)

Comment 7

2 years ago
Created attachment 8747836 [details] [diff] [review]
part 1 with -w
(Assignee)

Comment 8

2 years ago
Created attachment 8747837 [details] [diff] [review]
part 2 with -w

Updated

2 years ago
Attachment #8747833 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8747834 - Flags: review?(jmathies) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1193bfa38fdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/b65089fc5dac

Updated

2 years ago
Blocks: 1249209
tracking-e10s: ? → +
Priority: -- → P1

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1193bfa38fdd
https://hg.mozilla.org/mozilla-central/rev/b65089fc5dac
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Comment 11

2 years ago
Andrew, any interest in uplifting this to 48 or 47?
Flags: needinfo?(continuation)
(Assignee)

Comment 12

2 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

2 years ago
Attachment #8747834 - Flags: approval-mozilla-beta?
Attachment #8747834 - Flags: approval-mozilla-aurora?

Updated

2 years ago
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+

Updated

2 years ago
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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6087393c8970
https://hg.mozilla.org/releases/mozilla-aurora/rev/598c9eb576ab
status-firefox48: affected → fixed
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

2 years ago
Created attachment 8751841 [details] [diff] [review]
(for beta) part 1 - Use early continue in TransferableToIPCTransferable.
(Assignee)

Comment 17

2 years ago
Created attachment 8751842 [details] [diff] [review]
(for beta) part 2 - Null check first argument to nsContentUtils::GetSurfaceData().
(Assignee)

Comment 18

2 years ago
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
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.