First crash for 82.0a1 has build id 20200829091226, for 80 beta first affected is 81.0b4. Is there a change which got uplifted for beta 4 (or 3) and landed shortly before on central?

Crash report: https://crash-stats.mozilla.org/report/index/2ac9de80-6106-4a83-8cfa-318880200830

Top 10 frames of crashing thread:

0 xul.dll mozilla::WeakPtr<mozilla::layers::PersistentBufferProvider>::operator= mfbt/WeakPtr.h:257
1 xul.dll mozilla::layers::BorrowedSourceSurface::BorrowedSourceSurface gfx/layers/CanvasRenderer.cpp:27
2 xul.dll mozilla::layers::CanvasRenderer::BorrowSnapshot const gfx/layers/CanvasRenderer.cpp:66
3 xul.dll mozilla::layers::ShareableCanvasRenderer::UpdateCompositableClient gfx/layers/ShareableCanvasRenderer.cpp:189
4 xul.dll nsDisplayCanvas::CreateWebRenderCommands layout/generic/nsHTMLCanvasFrame.cpp:144
5 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommands gfx/layers/wr/WebRenderCommandBuilder.cpp:1653
6 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1758
7 xul.dll nsDisplayOwnLayer::CreateWebRenderCommands layout/painting/nsDisplayList.cpp:6335
8 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommands gfx/layers/wr/WebRenderCommandBuilder.cpp:1653
9 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1758

Not sure about the uplift timeline discrepancy but this code looks like it was introduced in bug 1632249. Redirecting to jgilbert.

It's bug 1654211.

(In reply to Jeff Gilbert [:jgilbert] from comment #2)

It's bug 1654211.

What do you mean? Is this bug duplicate of bug 1654211, or caused by bug 1654211?

This bug is a regression due to bug 1654211, as I labeled them.

(In reply to Jeff Gilbert [:jgilbert] from comment #4)

This bug is a regression due to bug 1654211, as I labeled them.

If you mean the "regressed by" field, it does not show anything for people who are not cc'd on that bug.

(In reply to Miko Mynttinen [:miko] from comment #5)

(In reply to Jeff Gilbert [:jgilbert] from comment #4)

This bug is a regression due to bug 1654211, as I labeled them.

If you mean the "regressed by" field, it does not show anything for people who are not cc'd on that bug.

Oh right, of course. :S

Ok so here we're assigning a dead (UAF) object into a WeakPtr, so we started using WeakPtr too late.

Jeff, any update?

My theory is that we're taking a reference to provider, but that may get changed out if provider->BorrowSnapshot() fails and we call context->GetFrontBufferSnapshot(...).
We need a strongref to provider instead, and we also need to null provider if BorrowSnapshot fails, since we're not borrowing in that case.

Also null out provider if we failed to borrow from the provider.

Comment on attachment 9179389 [details]
Bug 1664257 - Hold a ref to BufferProvider in CanvasRenderer::BorrowSnapshot.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't think an exploit is viable.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 82+
• If not all supported branches, which bug introduced the flaw?: Bug 1654211
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions, since things should only change when we would be hitting this UAF, and this patch should prevent the UAF.

Comment on attachment 9179389 [details]
Bug 1664257 - Hold a ref to BufferProvider in CanvasRenderer::BorrowSnapshot.

Beta/Release Uplift Approval Request

• User impact if declined: sec-high
• 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 should just make things better, but it does mess with lifetimes a little.
• String changes made/needed: none

https://hg.mozilla.org/integration/autoland/rev/87003c2d796d2518ae4723a62dd6cf6f28781a7c

Comment on attachment 9179389 [details]
Bug 1664257 - Hold a ref to BufferProvider in CanvasRenderer::BorrowSnapshot.

approved for 82.0b9

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

https://hg.mozilla.org/mozilla-central/rev/87003c2d796d

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.