Closed
Bug 1363677
Opened 7 years ago
Closed 7 years ago
Do not process old DeviceContext's draw command after device reset
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: kechen, Assigned: kechen)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
dvander
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/41d11d56768c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 8•7 years 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•7 years ago
|
status-firefox54:
--- → affected
Comment 9•7 years 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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/252e9fecd135
Comment 11•7 years ago
|
||
(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.
Description
•