Crash in [@ mozilla::gfx::AdjustedPattern::operator mozilla::gfx::Pattern*]
Categories
(Core :: Graphics, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox72 | --- | unaffected |
firefox73 | blocking | verified |
firefox74 | + | verified |
People
(Reporter: marcia, Assigned: bobowen)
References
(Regression)
Details
(Keywords: crash, regression, topcrash)
Crash Data
Attachments
(4 files)
This bug is for crash report bp-73596e27-d2c3-4bf6-b649-6e1e80191214.
Seen while looking at nightly crash stats - crashes started in 20191213094653: https://bit.ly/2rYOmxp
Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=192e0e33eb597e8d923eb89f6d49bf42654e9d11&tochange=f09f24f2b545919ab0db4bd352c276779c58bcc6
Top 10 frames of crashing thread:
0 XUL mozilla::gfx::AdjustedPattern::operator mozilla::gfx::Pattern* gfx/2d/DrawTargetWrapAndRecord.cpp:239
1 XUL mozilla::gfx::DrawTargetWrapAndRecord::FillRect gfx/2d/DrawTargetWrapAndRecord.cpp:309
2 XUL gfxSurfaceDrawable::DrawInternal gfx/thebes/gfxDrawable.cpp
3 XUL gfxSurfaceDrawable::Draw gfx/thebes/gfxDrawable.cpp:64
4 XUL gfxUtils::DrawPixelSnapped gfx/thebes/gfxUtils.cpp:562
5 XUL mozilla::image::RasterImage::DrawInternal image/RasterImage.cpp:1380
6 XUL mozilla::image::RasterImage::Draw image/RasterImage.cpp:1442
7 XUL DrawImageInternal layout/base/nsLayoutUtils.cpp:6872
8 XUL nsLayoutUtils::DrawSingleImage layout/base/nsLayoutUtils.cpp:6957
9 XUL nsImageFrame::PaintImage layout/generic/nsImageFrame.cpp:2010
Comment 1•3 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Comment 2•3 years ago
|
||
Cameron: could this be related to the changes for svg theme switching?
https://hg.mozilla.org/mozilla-central/rev/5010eb676e049f8926a798b0e2de1a8e8b2456e5
Updated•3 years ago
|
Comment 3•3 years ago
|
||
[Tracking Requested - why for this release]:
this signature is quite visible in the early rollout of 73.0b (3% of content crashes).
Comment 4•3 years ago
|
||
Raising the priority of this issue due to crashes in 73 beta
Comment 5•3 years ago
|
||
Maybe but I don't see anything specific to SVG images in the stack traces, and all of them seem to be happening during printing. I know that there were some printing changes around the same time, e.g. bug 1552966, though I don't see it in the possible regression range. Jonathan, do you think it's possible that bug is related?
Other than that, I don't see anything else related in the possible regression range, so if it's not a printing-related regression, I'm happy for bug 1598480 to be backed out to see if it helps.
Comment 7•3 years ago
|
||
Bug 1552966 got backed out so can't be that.
Updated•3 years ago
|
Comment 8•3 years ago
|
||
Tried downloading the current beta and reproducing by printing some of the URLs from crash reports to no avail.
Comment 9•3 years ago
|
||
Not sure if beta approval is needed for a backout, but here's one anyway. If this resolves the crashes I'll back out the patch from mozilla-central too.
Approval Request Comment
[Feature/Bug causing the regression]: bug 1598480 (maybe!)
[User impact if declined]: possible crash during printing
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no, crash is not yet reproducable
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: no
[Why is the change risky/not risky?]: cleanly applied backout of another bug
[String changes made/needed]: -
Comment 10•3 years ago
|
||
I'm going to clear the approval request so this doesn't show up on the needs-uplift radar, but taking the backout for 73.0b3 to see how it affects the crash rate.
Updated•3 years ago
|
Comment 11•3 years ago
|
||
uplift |
Backout pushed to Beta. Leaving the status set to affected until we can see what the crash rate looks like for b3+.
https://hg.mozilla.org/releases/mozilla-beta/rev/d147b9ba20b2
Comment 12•3 years ago
|
||
unfortunately this crash seems to continue in 73.0b3 after the backout.
Updated•3 years ago
|
Comment 13•3 years ago
|
||
This still potentially looks printing related, Layout friends - can ya'll take a look?
Comment 14•3 years ago
|
||
Re-adding jwatt in case he has any other ideas ... I think we backed out his other patches in time for b3 though, so I'm not sure what remains.
Comment 15•3 years ago
|
||
Bumping the severity of this to blocking given the crash rate.
Comment 16•3 years ago
•
|
||
(In reply to Jessie [:jbonisteel] plz needinfo from comment #13)
This still potentially looks printing related, Layout friends - can ya'll take a look?
yes, one comment said "WHEN I GO TO PRINT IT CRASHES" and the top crashing url is about:printpreview followed by some other pages with receipts, tickets etc that people are probably trying to print.
the url pattern is also looking a bit similar to bug 1607671 (which svoisen was referring to) in that it predominantly contains indian sites/domains.
the first crashing nightly build recorded now was 20191212095326, the only obvious printing related change there would have been bug 1602561:
![]() |
||
Comment 17•3 years ago
|
||
Ryan, can we back out bug 1486968 (in backs out cleanly)? It has the potential to have weird lifetime side affects.
![]() |
||
Updated•3 years ago
|
![]() |
||
Comment 18•3 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #8)
Tried downloading the current beta and reproducing by printing some of the URLs from crash reports to no avail.
I've managed to reproduce this crash once with https://www.knowyourgst.com/gst-number-search/ while testing various URLs. So far I haven't got it to crash again with that URL with a debugger attached though.
Comment 19•3 years ago
|
||
backout |
(In reply to Jonathan Watt [:jwatt] from comment #17)
Ryan, can we back out bug 1486968 (in backs out cleanly)? It has the potential to have weird lifetime side affects.
Pushed to Beta for 73.0b6:
https://hg.mozilla.org/releases/mozilla-beta/rev/00808d720d1b9a942a8e5f68aa9f6019367b18a0
Comment 20•3 years ago
|
||
it's still very early in the rollout phase for 73.0b6 but we're getting some of those crash reports with the same stack already.
are there any more ideas off-hand that we could try addressing this problem?
![]() |
||
Comment 21•3 years ago
|
||
:( As far as backouts go bug 1602561 is in range, although I'm not sure how that could possibly cause this. As far as a patch goes I'm still working on it but our glacial Windows build setup, unnusual amounts of breakage around that commit range and difficulty in reproducing the crash is making for slow going.
![]() |
||
Comment 22•3 years ago
•
|
||
(In reply to Marcia Knous [:marcia] from comment #0)
Top 10 frames of crashing thread:
0 XUL mozilla::gfx::AdjustedPattern::operator mozilla::gfx::Pattern* gfx/2d/DrawTargetWrapAndRecord.cpp:239
1 XUL mozilla::gfx::DrawTargetWrapAndRecord::FillRect gfx/2d/DrawTargetWrapAndRecord.cpp:309
[snip]
Things go bad in the GetSourceSurface call made by AdjustedPattern::operator Pattern*()
above. GetSourceSurface casts to SourceSurfaceWrapAndRecord
if the surface type is SurfaceType::RECORDING
, but multiple C++ types claim to be of type SurfaceType::RECORDING
. In the case of this crash, the surface is actually a SourceSurfaceRecording
(another type claiming to be SurfaceType::RECORDING
is DataSourceSurfaceRecording
). The invalid pointer returned by GetSourceSurface
is then addref'ed in the SurfacePattern constructor when it is assigned to the RefPtr mSurface, and we crash.
![]() |
||
Comment 23•3 years ago
|
||
Jeff, I don't know which change has triggered this, but this is clearly a pre-existing bug. Is someone on gfx able to fix the issue with GetSourceSurface
?
Comment 24•3 years ago
|
||
Given where we are in the cycle (All Hands next week, RC the week after that), we're very quickly running out of time to address this bug this cycle. Can we please find someone to own this blocking topcrash bug?
Updated•3 years ago
|
Comment 25•3 years ago
|
||
FTR, this is currently the #1 overall topcrash on Beta.
Comment 26•3 years ago
|
||
This was probably caused by bug 1598582.
Updated•3 years ago
|
Comment 27•3 years ago
|
||
For now we can probably solve this issue by backing out the two changesets from 1598582. I confirmed with Bob that those changes only help with canvas remoting which is not on in beta so there should be no negative side-effects.
Assignee | ||
Comment 28•3 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #27)
For now we can probably solve this issue by backing out the two changesets from 1598582. I confirmed with Bob that those changes only help with canvas remoting which is not on in beta so there should be no negative side-effects.
We'd need to back out bug 1604180 as well, which fixed a different regression from bug 1598582.
Comment 29•3 years ago
|
||
This reverts commits 5ba85903b6765bcb75b0f945e2833da1c178d797,
908c5b3e65b491cfcd71acedc853f989191a3e22 and
593bc922d369bab405331fc9a5496d21303bdf2b for causing some type confusion
crashes.
Comment 30•3 years ago
|
||
Comment on attachment 9122105 [details]
Bug 1604800. Revert Bug 1604180 and Bug 1598582.
Beta/Release Uplift Approval Request
- User impact if declined: Crashes during printing
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Medium
- Why is the change risky/not risky? (and alternatives if risky): This is just backing out some changes that caused that we expect caused this problem.
- String changes made/needed:
Updated•3 years ago
|
Updated•3 years ago
|
Comment 31•3 years ago
|
||
Comment on attachment 9122105 [details]
Bug 1604800. Revert Bug 1604180 and Bug 1598582.
Reverts changes landed in Fx73 that we suspect are causing this crash. Approved for 73.0b8.
Comment 32•3 years ago
|
||
bugherderuplift |
Comment 33•3 years ago
|
||
Let's leave 73 set to affected until we can verify via crash-stats that the backout worked. I'll remove the approval flag to get the patch off the needs-uplift radar in the mean time.
Updated•3 years ago
|
Comment 34•3 years ago
|
||
Also adding qe-verify+ to this bug so QA can also monitor if the crash still happens after a few betas.
Comment 35•3 years ago
|
||
No crashes in 73.0b8, looks like backout #3 worked!
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 37•3 years ago
|
||
I'll take this now for a fix on m-c (74+) without backing out those other changes.
(jrmuizel - thanks again for sorting the backout.)
Assignee | ||
Comment 38•3 years ago
|
||
This is to avoid type confusion with SourceSurfaceRecording.
Assignee | ||
Comment 39•3 years ago
|
||
This is done by using CreateWrappingDataSourceSurface to create a SourceSurface
and then OptimizeSourceSurface to record and optimize that.
This means that we now only have SourceSurfaceRecording using SurfaceType
RECORDING. We then rely on this to improve the test in OptimizeSourceSurface to
check that a SourceSurfaceRecording is for its recorder.
Depends on D60735
Assignee | ||
Comment 40•3 years ago
|
||
Comment 41•3 years ago
|
||
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/autoland/rev/660737718c70 Part 1: Add a new WRAP_AND_RECORD SurfaceType for SourceSurfaceWrapAndRecord. r=jrmuizel https://hg.mozilla.org/integration/autoland/rev/2a10ae0264cb Part 2: Remove DataSourceSurfaceRecording from DrawTargetRecording. r=jrmuizel
Comment 42•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/660737718c70
https://hg.mozilla.org/mozilla-central/rev/2a10ae0264cb
Comment 43•3 years ago
|
||
Based on the lack of crash reports wit the same signature following the fix, I deem this fix verified in Nightly channel as well.
Comment 44•2 years ago
|
||
Please specify a root cause for this bug. See :tmaity for more information.
Assignee | ||
Updated•2 years ago
|
Description
•