Closed Bug 1604800 Opened 1 year ago Closed 1 year ago

Crash in [@ mozilla::gfx::AdjustedPattern::operator mozilla::gfx::Pattern*]

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla74
Project Flags:
Root Cause Coding: Internal API Issue
Tracking Flags:
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)

beta backout patch
26.37 KB, patch
Details | Diff | Splinter Review
Bug 1604800. Revert Bug 1604180 and Bug 1598582.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1604800 Part 1: Add a new WRAP_AND_RECORD SurfaceType for SourceSurfaceWrapAndRecord. r=jrmuizel!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1604800 Part 2: Remove DataSourceSurfaceRecording from DrawTargetRecording. r=jrmuizel!
47 bytes, text/x-phabricator-request
Details | Review

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Cameron: could this be related to the changes for svg theme switching?
https://hg.mozilla.org/mozilla-central/rev/5010eb676e049f8926a798b0e2de1a8e8b2456e5

Flags: needinfo?(cam)
Priority: -- → P3

[Tracking Requested - why for this release]:
this signature is quite visible in the early rollout of 73.0b (3% of content crashes).

status-firefox74: --- → affected
status-firefox-esr68: --- → unaffected
tracking-firefox73: --- → ?

Raising the priority of this issue due to crashes in 73 beta

Priority: P3 → P1

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.

Flags: needinfo?(cam)

(see comment 5)

Flags: needinfo?(jwatt)

Bug 1552966 got backed out so can't be that.

Flags: needinfo?(jwatt)

Tried downloading the current beta and reproducing by printing some of the URLs from crash reports to no avail.

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]: -

Attachment #9119612 - Flags: approval-mozilla-beta?

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.

tracking-firefox73: ? → +
tracking-firefox74: --- → +
Attachment #9119612 - Flags: approval-mozilla-beta?

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

unfortunately this crash seems to continue in 73.0b3 after the backout.

Flags: needinfo?(svoisen)

This still potentially looks printing related, Layout friends - can ya'll take a look?

Flags: needinfo?(jfkthame)

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.

Flags: needinfo?(svoisen) → needinfo?(jwatt)

Bumping the severity of this to blocking given the crash rate.

Severity: normal → critical
tracking-firefox73: + → blocking

(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:

Ryan, can we back out bug 1486968 (in backs out cleanly)? It has the potential to have weird lifetime side affects.

Flags: needinfo?(jwatt) → needinfo?(ryanvm)

(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.

(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

Flags: needinfo?(ryanvm)

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?

:( 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.

(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.

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?

Flags: needinfo?(jmuizelaar)

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?

Flags: needinfo?(mreavy)
Assignee: nobody → jmuizelaar
Flags: needinfo?(mreavy)

FTR, this is currently the #1 overall topcrash on Beta.

This was probably caused by bug 1598582.

Flags: needinfo?(jmuizelaar)
Regressed by: 1598582

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.

(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.

This reverts commits 5ba85903b6765bcb75b0f945e2833da1c178d797,
908c5b3e65b491cfcd71acedc853f989191a3e22 and
593bc922d369bab405331fc9a5496d21303bdf2b for causing some type confusion
crashes.

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:
Attachment #9122105 - Flags: approval-mozilla-beta?

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.

Attachment #9122105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

status-firefox73: fixed → affected
Attachment #9122105 - Flags: approval-mozilla-beta+

Also adding qe-verify+ to this bug so QA can also monitor if the crash still happens after a few betas.

Flags: qe-verify+
See Also: → 1610859

No crashes in 73.0b8, looks like backout #3 worked!

status-firefox73: affected → verified
Duplicate of this bug: 1610859
Flags: needinfo?(jfkthame)

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: jmuizelaar → bobowencode
Status: NEW → ASSIGNED

This is to avoid type confusion with SourceSurfaceRecording.

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

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

https://hg.mozilla.org/mozilla-central/rev/660737718c70
https://hg.mozilla.org/mozilla-central/rev/2a10ae0264cb

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox74: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

Please specify a root cause for this bug. See :tmaity for more information.

Root Cause: --- → ?
Root Cause: ? → Coding: Internal API Issue
You need to log in before you can comment on or make changes to this bug.