Closed Bug 1356119 Opened 7 years ago Closed 7 years ago

Do not Flush() old context after device reset

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: kechen, Assigned: kechen)

References

Details

Attachments

(2 files, 1 obsolete file)

Skip Flush to old context after device reset;
3.31 KB, patch
kechen
: review+
Details | Diff | Splinter Review
[Uplift] Skip Flush to old context after device reset;
2.50 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Per Bug 1351349 comment 7, I think we should not call Flush() to an old context since it might cause crashes.
Blocks: 1351349
Hello David,

Could you help me to confirm the bug and review this patch ? I am not sure if this patch can actually solve the problem rather than redirect the crash to another crash signature.
Flags: needinfo?(dvander)
Comment on attachment 8857771 [details]
Bug 1356119 - Skip Flush to old context after device reset;

I honestly don't know if this will help. It's worth trying and seeing if this crash goes away and another one spikes. We can always back it out later.
Flags: needinfo?(dvander)
Attachment #8857771 - Flags: review?(dvander) → review+
Keywords: checkin-needed
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac3a94bbc87
Skip Flush to old context after device reset. r=dvander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ac3a94bbc87
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Approval Request Comment
[Feature/Bug causing the regression]:
  This part of code is added by Bug 1300121
[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?]:
  Yes.
[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 really risky.
[Why is the change risky/not risky?]:
  This part of code can be only involved when device reset happened which is rare. And the fix skip the flush command to a invalid context which should be fine.
[String changes made/needed]:
  None.
Attachment #8859831 - Flags: approval-mozilla-beta?
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #7)
> Created attachment 8859831 [details] [diff] [review]
> ...
> [Has the fix been verified in Nightly?]:
>   Yes.

How?  Have we seen some crash statistic to support this?
Flags: needinfo?(kechen)
(In reply to Milan Sreckovic [:milan] from comment #8)
> (In reply to Kevin Chen[:kechen] (UTC + 8) from comment #7)
> > Created attachment 8859831 [details] [diff] [review]
> > ...
> > [Has the fix been verified in Nightly?]:
> >   Yes.
> 
> How?  Have we seen some crash statistic to support this?

There were no more crashes in the nightly channel in Bug 1351349 since the fix. I was expecting the crash would still happen with different entry point if the fix doesn't work.

But since the sample number in the nightly might not enough to support this and I cannot ensure that this part of the code is really tested, I will change this to "No".

Thank you for the reminding.
Flags: needinfo?(kechen)
Comment on attachment 8859831 [details] [diff] [review]
[Uplift] Skip Flush to old context after device reset;

Rewrite the request comment to avoid the confusion.

Approval Request Comment
[Feature/Bug causing the regression]:
  This part of code is added by Bug 1300121
[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.
Comment on attachment 8859831 [details] [diff] [review]
[Uplift] Skip Flush to old context after device reset;

try to avoid crashes on gpu reset, beta54+

Should be in 54b3
Attachment #8859831 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #11)
> [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-
See Also: → 1363677
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: