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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: kechen, Assigned: kechen)

References

Details

Attachments

(1 file)

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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/41d11d56768c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
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+
(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.

Attachment

General

Created:
Updated:
Size: