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

VERIFIED FIXED in Firefox 35

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

({crash})

unspecified
mozilla38
x86
Windows NT
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox35+ verified, firefox36 verified, firefox37 verified, firefox38 verified, relnote-firefox 35+)

Details

(crash signature)

Attachments

(1 attachment)

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

Null D2DFactory() at DrawTargetD2D1::factory().
[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).
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?
Attachment #8550074 - Flags: review?(bas)
Startup crash, tracking.
Assigning to remove it from incoming gfx triage
Assignee: nobody → dmajor
[Tracking Requested - why for this release]: Doh, I meant to request tracking for 35, not 36. Please see comment 1.
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.
Attachment #8550074 - Flags: review?(bas) → review+
Tracking as a potential ride-along, though how certain can we be that this null check is going to resolve the issue?
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.
> 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?
Flags: needinfo?(bas)
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.
Attachment #8550074 - Flags: approval-mozilla-release?
Attachment #8550074 - Flags: approval-mozilla-beta?
Attachment #8550074 - Flags: approval-mozilla-aurora?
David, the bug is marked as not affecting 36, I guess you can to merge it to be safe, right?
https://hg.mozilla.org/mozilla-central/rev/0cd5c5fb458e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8550074 [details] [diff] [review]
Null check bandaid

Taking it for a potential 35.0.1
Attachment #8550074 - Flags: approval-mozilla-release? → approval-mozilla-release+
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.
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.
Attachment #8550074 - Flags: approval-mozilla-beta?
Attachment #8550074 - Flags: approval-mozilla-beta+
Attachment #8550074 - Flags: approval-mozilla-aurora?
Attachment #8550074 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
(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).
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.