1418791 - Crash in mozilla::gfx::FillRectCommand::FillRectCommand

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[[:philipp]]

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.

Comment 1

[Peter Chang[:pchang]]

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

Comment 2

[Peter Chang[:pchang]]

From these crash reports, they all enabled OMTP by default.

Comment 3

[Peter Chang[:pchang]]

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

Comment 4

[Peter Chang[:pchang]]

Attachment: Bug 1418791 - Ensure mSnapshot access is protected by mutex,

Review commit: https://reviewboard.mozilla.org/r/202406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202406/

Comment 5

[Sotaro Ikeda [:sotaro]]

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.

Comment 6

[Peter Chang[:pchang]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fc4a6a0520b
Explicit free the resource of PersistentBufferProviderBasic, r=sotaro

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4fc4a6a0520b

Comment 9

[Peter Chang[:pchang]]

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

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8931260 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

Fix a crash. Beta58+.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7a7bffc4fdc

Comment 12

[Andrei Vaida [:avaida]]

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

Comment 13

[Peter Chang[:pchang]]

Keep opening until I get the number from new beta release.

Comment 14

[[:philipp]]

the issue seems to be still ongoing in 58.0b7 which includes the patch.

Comment 15

[Peter Chang[:pchang]]

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

Comment 16

[Peter Chang[:pchang]]

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/

Comment 17

[Peter Chang[:pchang]]

(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

Comment 18

[Peter Chang[:pchang]]

Attachment: Bug 1418791 - Ensure mSnapshot access is protected by mutex,

Review commit: https://reviewboard.mozilla.org/r/205356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205356/

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

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!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f9148a1489b
Ensure mSnapshot access is protected by mutex, r=dvander

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6f9148a1489b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're ready to do so :)

Comment 23

[Peter Chang[:pchang]]

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

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8934427 [details]
Bug 1418791 - Ensure mSnapshot access is protected by mutex,

crash fix, beta58+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f36855025582