Crash in mozilla::gfx::FillRectCommand::FillRectCommand

RESOLVED FIXED in Firefox 58

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: pchang)

Tracking

({crash, regression})

58 Branch
mozilla59
x86_64
Windows
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 fixed, firefox59 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

This bug was filed from the Socorro interface and is
report bp-bae3f067-0dcb-45ea-b6fb-1fd380171119.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::gfx::FillRectCommand::FillRectCommand gfx/2d/DrawCommand.h:328
1 xul.dll mozilla::layers::FillRectWithMask gfx/layers/basic/BasicLayersImpl.cpp:148
2 xul.dll mozilla::layers::FillRectWithMask gfx/layers/basic/BasicLayersImpl.cpp:172
3 xul.dll mozilla::layers::BasicCanvasLayer::Paint gfx/layers/basic/BasicCanvasLayer.cpp:66
4 xul.dll mozilla::layers::BasicLayerManager::PaintSelfOrChildren gfx/layers/basic/BasicLayerManager.cpp:711
5 xul.dll mozilla::layers::BasicLayerManager::PaintLayer gfx/layers/basic/BasicLayerManager.cpp:891
6 xul.dll mozilla::layers::BasicLayerManager::EndTransactionInternal gfx/layers/basic/BasicLayerManager.cpp:616
7 xul.dll mozilla::PaintInactiveLayer layout/painting/FrameLayerBuilder.cpp:3706
8 xul.dll mozilla::FrameLayerBuilder::PaintItems layout/painting/FrameLayerBuilder.cpp:6020
9 xul.dll mozilla::FrameLayerBuilder::DrawPaintedLayer layout/painting/FrameLayerBuilder.cpp:6200

=============================================================

crash reports in the content process on win64 builds are newly appearing in firefox 58. the ones were recorded on 58.01 build 20171103003834.
From the dump file, the address of surface[1] was valid but its content was e5e5e5e5. It means the memory of this sourcesurfe got freed. And then it caused the crash at [2].


[1]https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/layers/basic/BasicCanvasLayer.cpp#68
[2]https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/gfx/2d/DrawCommand.h#113
From these crash reports, they all enabled OMTP by default.
In BasicCanvasLayers[1], it tried to get the surface from CopyableCanvasRenderer->GetBufferProvider(). 
I think this provider was mapped to the PersistentBufferProviderBasic. Inside the PersistentBufferProviderBasic, it doesn't deal with the mDrawTarget deletion[3] and might cause the mapped buffer got freed in as mentioned in comment 1.

[1]https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/gfx/layers/basic/BasicCanvasLayer.cpp#48
[2]https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/gfx/layers/CopyableCanvasRenderer.cpp#102
[3]https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/gfx/layers/PersistentBufferProvider.h#108
Assignee: nobody → howareyou322
Crash Signature: [@ mozilla::gfx::FillRectCommand::FillRectCommand] → [@ mozilla::gfx::FillRectCommand::FillRectCommand] [@ mozilla::gfx::StoredPattern::Assign]
Comment on attachment 8931260 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

https://reviewboard.mozilla.org/r/202406/#review208014

Patch looks good, but it is not clear enough for me if the patch address the crash. It might better to check if the patch addresses the crash after commit.
Attachment #8931260 - Flags: review?(sotaro.ikeda.g) → review+
Try looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba123b35a3ba82e4a3b43976919bf530b706b680

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> Comment on attachment 8931260 [details]
> Bug 1418791 - Explicit free the resource of PersistentBufferProviderBasic,
> 
> https://reviewboard.mozilla.org/r/202406/#review208014
> 
> Patch looks good, but it is not clear enough for me if the patch address the
> crash. It might better to check if the patch addresses the crash after
> commit.
Sure, I will keep monitoring this bug.
Keywords: leave-open
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fc4a6a0520b
Explicit free the resource of PersistentBufferProviderBasic, r=sotaro
Comment on attachment 8931260 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

Approval Request Comment
[Feature/Bug causing the regression]: Related to OMTP
[User impact if declined]: Have chances to hit content process crashes
[Is this code covered by automated tests?]: Haven't seen any problem with try
[Has the fix been verified in Nightly?]: 
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: It explicit frees the resoruce to prevent these resource being used after free
[String changes made/needed]: None
Attachment #8931260 - Flags: approval-mozilla-beta?
Comment on attachment 8931260 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

Fix a crash. Beta58+.
Attachment #8931260 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Peter Chang[:pchang] from comment #9)
> [Is this code covered by automated tests?]: Haven't seen any problem with try
> [Has the fix been verified in Nightly?]: 
> [Needs manual test from QE? If yes, steps to reproduce]: No

Has automated coverage and does not need manual testing, per Peter.
Flags: qe-verify-
Keep opening until I get the number from new beta release.
the issue seems to be still ongoing in 58.0b7 which includes the patch.
Flags: needinfo?(howareyou322)
(In reply to [:philipp] from comment #14)
> the issue seems to be still ongoing in 58.0b7 which includes the patch.

Yes, but the crashes are smaller. I will check the new crash report to see any difference.
Flags: needinfo?(howareyou322)
(In reply to Peter Chang[:pchang] from comment #16)
> Comment on attachment 8931260 [details]
> Bug 1418791 - Ensure mSnapshot access is protected by mutex,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/202406/diff/1-2/

Looks like we are using SourceSurfaceD2D1 from DrawTargetD2D1 on windows and we don't guarantee sourcesurface are still valid from DrawTargetD2D1::Sanpshot calls[1].

[1]https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/gfx/2d/DrawTargetD2D1.cpp#97
Attachment #8931260 - Attachment is obsolete: true
Attachment #8931260 - Flags: review?(dvander)
Comment on attachment 8934427 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

https://reviewboard.mozilla.org/r/205356/#review210952

Whoops, good catch. Thanks!
Attachment #8934427 - Flags: review?(dvander) → review+
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f9148a1489b
Ensure mSnapshot access is protected by mutex, r=dvander
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Please request Beta approval on this when you're ready to do so :)
Flags: needinfo?(howareyou322)
Target Milestone: --- → mozilla59
Comment on attachment 8934427 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

Approval Request Comment
[Feature/Bug causing the regression]: related to OMTP
[User impact if declined]: Have possibilities to get content crashes
[Is this code covered by automated tests?]: Passed on try and was landed on central
[Has the fix been verified in Nightly?]: No, because we don't know the STR
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: The changes are added to protect the shared resource which is used by different threads and the risk level should be low.
[String changes made/needed]: No
Flags: needinfo?(howareyou322)
Attachment #8934427 - Flags: approval-mozilla-beta?
Comment on attachment 8934427 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

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