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

Status: RESOLVED FIXED

(Core :: Widget, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Maybe GetDataSurface() can return null sometimes?

Comment 1

[Andrew McCreight [:mccr8]]

I don't see a lot of these, but it could be an easy small win for e10s stability.

Comment 2

[Andrew McCreight [:mccr8]]

I'll just fix this. I think just returning failure if GetDataSurface() returned null should do it.

Comment 3

[Nicholas Nethercote [inactive]]

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

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Null check first argument to nsContentUtils::GetSurfaceData().

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 1 with -w

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 2 with -w

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1193bfa38fdd
https://hg.mozilla.org/mozilla-central/rev/b65089fc5dac

Comment 11

[Jim Mathies [:jimm]]

Andrew, any interest in uplifting this to 48 or 47?

Comment 12

[Andrew McCreight [:mccr8]]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8747833 [details] [diff] [review]
part 1 - Use early continue in TransferableToIPCTransferable.

Crash fix, Aurora48+, Beta47+

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6087393c8970
https://hg.mozilla.org/releases/mozilla-aurora/rev/598c9eb576ab

Comment 15

[Carsten Book [:Tomcat]]

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')

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: (for beta) part 1 - Use early continue in TransferableToIPCTransferable.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: (for beta) part 2 - Null check first argument to nsContentUtils::GetSurfaceData().

Comment 18

[Andrew McCreight [:mccr8]]

I rebased these for beta. Thanks!

Comment 19

[Carsten Book [:Tomcat]]

(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