If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Do not process old DeviceContext's draw command after device reset

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: kechen, Assigned: kechen)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
Per bug 1351349 comment 14, there is a good possibility that application crashes while  executing draw commands to an old DeviceContext after device reset.
Comment hidden (mozreview-request)

Comment 2

4 months ago
mozreview-review
Comment on attachment 8866648 [details]
Bug 1363677 - Skip Flush() and EndDraw() if the ID2D1DeviceContext is stale;

https://reviewboard.mozilla.org/r/138248/#review141802

::: gfx/2d/DrawTargetD2D1.h:297
(Diff revision 1)
>    bool mDidComplexBlendWithListInList;
>  
>    static ID2D1Factory1 *mFactory;
>    static IDWriteFactory *mDWriteFactory;
> +  // This value is uesed to verify if the DrawTarget is created by a stale device.
> +  ID2D1Device* mDevice;

I think this has to be a RefPtr, otherwise you have an A-B-A problem. If you're worried about keeping the old device alive you can use a generation counter.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

4 months ago
mozreview-review
Comment on attachment 8866648 [details]
Bug 1363677 - Skip Flush() and EndDraw() if the ID2D1DeviceContext is stale;

https://reviewboard.mozilla.org/r/138248/#review142170
Attachment #8866648 - Flags: review?(dvander) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 6

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/41d11d56768c
Skip Flush() and EndDraw() if the ID2D1DeviceContext is stale; r=dvander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/41d11d56768c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 8

4 months ago
Comment on attachment 8866648 [details]
Bug 1363677 - Skip Flush() and EndDraw() if the ID2D1DeviceContext is stale;

Approval Request Comment

[Feature/Bug causing the regression]:
  Not a regression, this code exists for a while.
[User impact if declined]:
  May run into the crash in Bug 1351349. 
[Is this code covered by automated tests?]:
  No.
[Has the fix been verified in Nightly?]:
  No.
  It is landed to Nightly for few days, and no related crash is found so far, but we could not ensure the crash scenario is tested since the user number is fewer in Nightly channel.
[Needs manual test from QE? If yes, steps to reproduce]: 
  No.
[List of other uplifts needed for the feature/fix]:
  None.
[Is the change risky?]:
  Not risky.
[Why is the change risky/not risky?]:
  This part of code can be only involved when device reset happened which is a rare case. And the fix skip the flush command to a invalid context which should be fine.
[String changes made/needed]:
  None.
Attachment #8866648 - Flags: approval-mozilla-beta?

Updated

4 months ago
status-firefox54: --- → affected

Comment 9

4 months ago
Comment on attachment 8866648 [details]
Bug 1363677 - Skip Flush() and EndDraw() if the ID2D1DeviceContext is stale;

Take this and see how it goes. Beta54+. Should be in 54 beta 9.
Attachment #8866648 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/252e9fecd135
status-firefox54: affected → fixed
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #8)
> [Is this code covered by automated tests?]:
>   No.
> [Has the fix been verified in Nightly?]:
>   No.
>   It is landed to Nightly for few days, and no related crash is found so
> far, but we could not ensure the crash scenario is tested since the user
> number is fewer in Nightly channel.
> [Needs manual test from QE? If yes, steps to reproduce]: 
>   No.

Setting qe-verify- based on Kevin's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.