Closed Bug 1536229 Opened 1 year ago Closed 1 year ago

Crash in [@ mozilla::gfx::SourceSurfaceCapture::Resolve]

Categories

(Core :: Graphics, defect, P2, critical)

66 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: philipp, Assigned: bas.schouten)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-f54d39d5-f344-4378-bbc6-e9d290190318.

Top 10 frames of crashing thread:

0 xul.dll mozilla::gfx::SourceSurfaceCapture::Resolve gfx/2d/SourceSurfaceCapture.cpp:97
1 xul.dll static class sk_sp<SkImage> mozilla::gfx::GetSkImageForSurface gfx/2d/DrawTargetSkia.cpp:239
2 xul.dll mozilla::gfx::DrawTargetSkia::DrawSurface gfx/2d/DrawTargetSkia.cpp:633
3 xul.dll mozilla::gfx::DrawTargetSkia::FillRect gfx/2d/DrawTargetSkia.cpp:764
4 xul.dll void mozilla::gfx::DrawTargetCaptureImpl::ReplayToDrawTarget gfx/2d/DrawTargetCapture.cpp:315
5 xul.dll void mozilla::layers::PaintThread::AsyncPaintTask gfx/layers/PaintThread.cpp:203
6 xul.dll nsresult mozilla::detail::RunnableFunction<`lambda at z:/build/build/src/gfx/layers/PaintThread.cpp:174:30'>::Run xpcom/threads/nsThreadUtils.h:559
7 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:241
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1161
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:474

these windows content crash reports with MOZ_RELEASE_ASSERT(NS_IsMainThread() || !mOwner) are regressing in numbers since nightly 67.0a1 build 20190307215858 and on beta since 66.0rc1.

in this timeframe also some fixes to address other gfx crashes (bug 1526045, bug 1524554) had landed, so i presume this might be related...

Bas, could this be connected to the bugs mentioned above as well?

Flags: needinfo?(bas)
Priority: -- → P2

It certainly could.

Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)

Ryan, do you remember why this assert is there? It's not obvious to me why we assert that.

Flags: needinfo?(rhunt)

It was added by David to try to assert some thread safety around the COW command list. Essentially it asserts that the SourceSurfaceCapture that was created while recording a main thread paint has been disconnected from it's DrawTargetCapture that created it. This is to prevent that DrawTargetCapture from being used on the main thread, while this SourceSurfaceCapture is being replayed on the paint thread.

However we have a mutex around all these methods to try to ensure this thread safety as well. And I don't recall any guarantee that SourceSurfaceCapture's would be disconnected when sending them to the paint thread. So I'm fine with getting rid of it.

Bug 1533237 may also be relevant.

Flags: needinfo?(rhunt)

The volume on both release and beta is noticable, Bas, have you been able to investigate this issue?

Flags: needinfo?(bas)
Duplicate of this bug: 1445481

I will put up a patch for this somewhere this week!

Flags: needinfo?(bas)

(In reply to Bas Schouten (:bas.schouten) from comment #7)

I will put up a patch for this somewhere this week!

Any progress on this front? Thanks

Flags: needinfo?(bas)

Bas, it seems that this patch got r+ and is minimal, could you land it and request an uplift to our last beta? Thanks

Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d43e71f8770
Remove NS_RELEASE_ASSERT that is being tripped and seems to be unnecessary. r=rhunt
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Ryan, you reviewed the patch, could you make an uplift request today please? Thanks

Flags: needinfo?(rhunt)

Comment on attachment 9061199 [details]
Bug 1536229: Remove NS_RELEASE_ASSERT that is being tripped and seems to be unnecessary. r=rhunt

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Removing a release assert that we believe serves no purposes, and at the worst case seems like it should cause small and rare visual artifacts.
  • String changes made/needed: None
Flags: needinfo?(bas)
Attachment #9061199 - Flags: approval-mozilla-beta?
Flags: needinfo?(rhunt)

Comment on attachment 9061199 [details]
Bug 1536229: Remove NS_RELEASE_ASSERT that is being tripped and seems to be unnecessary. r=rhunt

OK for 67 uplift; this should make it into the RC build.

Attachment #9061199 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.