1122367 - Startup crash in IUnknown::QueryInterface<ID2D1Factory1>(ID2D1Factory1**)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-21472ff3-742c-4efe-ab1d-9ca8d2150113.
=============================================================

Null D2DFactory() at DrawTargetD2D1::factory().

Comment 1

[(Away)]

[Tracking Requested - why for this release]:

In early 35.0 data this is the #7 crash at 0.86%. That's a relatively low percentage, but as a persistent startup crash this will lose users.

No reports from 36 AFAICT (maybe that's due to other factors).

Comment 2

[(Away)]

Attachment: Null check bandaid

Offering a patch since it looks straightforward. I have no idea whether this merely causes the affected users to crash somewhere else instead.

I'm out of town over the weekend; if this needs to proceed could someone land it?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Startup crash, tracking.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Assigning to remove it from incoming gfx triage

Comment 5

[(Away)]

[Tracking Requested - why for this release]: Doh, I meant to request tracking for 35, not 36. Please see comment 1.

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8550074 [details] [diff] [review]
Null check bandaid

Review of attachment 8550074 [details] [diff] [review]:
-----------------------------------------------------------------

Technically this shouldn't be able to happen, but there's nothing wrong with this patch, so hey.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking as a potential ride-along, though how certain can we be that this null check is going to resolve the issue?

Comment 8

[(Away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd5c5fb458e

> Tracking as a potential ride-along, though how certain can we be that this
> null check is going to resolve the issue?

From a code perspective, I don't know at all (maybe the area experts might). From an empirical perspective, we haven't seen this crash anywhere but the 35 train, so we're unlikely to get meaningful data from the pre-release channels.

Comment 9

[(Away)]

> Technically this shouldn't be able to happen

I noticed that none of the crash reports have d2d1.dll loaded. Does that offer any clues?

Is it reasonable to assume that we won't try any further D2D1-ish things after SupportsD2D1() returns false? In other words, that we don't need to add a bunch of other null checks elsewhere?

Comment 10

[(Away)]

Comment on attachment 8550074 [details] [diff] [review]
Null check bandaid

Approval Request Comment
[Feature/regressing bug #]: Unknown
[User impact if declined]: Startup crashes on release
[Describe test coverage new/current, TBPL]: None. Speculative fix.
[Risks and why]: It is low risk in the sense that it won't affect the users who aren't crashing today. The biggest risk is if the fix is incomplete, and the crashing users just crash somewhere else.
[String/UUID change made/needed]: None.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

David, the bug is marked as not affecting 36, I guess you can to merge it to be safe, right?

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cd5c5fb458e

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8550074 [details] [diff] [review]
Null check bandaid

Taking it for a potential 35.0.1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/ca0831e0d7dc

Are 36/37 unaffected as in this bug really doesn't affect them or our test audiences are just too small for it to show up? I'm wondering why this is landing on trunk and release and not the two branches in between.

Comment 15

[(Away)]

I don't know why this doesn't affect 36 and 37. Crash-stats has only ever seen it on 35 betas and 35.0 release.

But we might as well merge to those branches for consistency. This patch will do zero harm to builds that aren't already crashing.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c640463ce6a
https://hg.mozilla.org/releases/mozilla-beta/rev/57cb206153af

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

No crashes in Socorro for 35.0.1 / 36 beta / 37 Aurora and 38 Nightly.

Marking verified but I'll keep an eye on this the next days to ensure this crash will not reappear on the radar.

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to :dmajor (back Feb 9) from comment #9)
> > Technically this shouldn't be able to happen
> 
> I noticed that none of the crash reports have d2d1.dll loaded. Does that
> offer any clues?
> 
> Is it reasonable to assume that we won't try any further D2D1-ish things
> after SupportsD2D1() returns false? In other words, that we don't need to
> add a bunch of other null checks elsewhere?

Nope, it's just weird. Even if we tried to do D2D1-ish things, that's bad, but if that was just the problem, this nullcheck shouldn't help. The added nullcheck is for the D2D 1.0 load (I realize this naming is all confusing).