1516787 - Crash in mozilla::gl::SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-8c3eb33c-b613-4d05-9b5b-eb2b30181228.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll void mozilla::gl::SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle gfx/gl/SharedSurfaceANGLE.cpp:101
1 xul.dll void mozilla::gl::SharedSurface_ANGLEShareHandle::~SharedSurface_ANGLEShareHandle gfx/gl/SharedSurfaceANGLE.cpp:100
2 xul.dll void mozilla::layers::SharedSurfaceTextureData::~SharedSurfaceTextureData gfx/layers/client/TextureClientSharedSurface.cpp:26
3 xul.dll void mozilla::layers::SharedSurfaceTextureClient::~SharedSurfaceTextureClient gfx/layers/client/TextureClientSharedSurface.cpp:80
4 xul.dll void mozilla::layers::SharedSurfaceTextureClient::~SharedSurfaceTextureClient gfx/layers/client/TextureClientSharedSurface.cpp:62
5 xul.dll mozilla::AtomicRefCountedWithFinalize<mozilla::layers::TextureClient>::Release gfx/layers/AtomicRefCountedWithFinalize.h:152
6 xul.dll void std::_List_buy<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> >, std::allocator<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> > > >::_Freenode vs2017_15.8.4/VC/include/list:728
7 xul.dll class std::_List_iterator<std::_List_val<std::_List_simple_types<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> > > > > std::_Hash<std::_Umap_traits<unsigned long long, RefPtr<mozilla::layers::TextureClient>, std::_Uhash_compare<unsigned long long, std::hash<unsigned long long>, std::equal_to<unsigned long long> >, std::allocator<std::pair<const unsigned long long, RefPtr<mozilla::layers::TextureClient> > >, 0> >::erase vs2017_15.8.4/VC/include/xhash:607
8 xul.dll void mozilla::layers::CompositorBridgeChild::NotifyNotUsed gfx/layers/ipc/CompositorBridgeChild.cpp:839
9 xul.dll mozilla::layers::CompositorBridgeChild::RecvParentAsyncMessages gfx/layers/ipc/CompositorBridgeChild.cpp:784

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


There are 4 crashes (from 4 installations) in nightly 66 starting with buildid 20181228093007. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1493497.

[1] https://hg.mozilla.org/mozilla-central/rev?node=41d9d168940d

Comment 1

[Sotaro Ikeda [:sotaro]]

I misunderstood mGL as the mGL is defined as strong reference. It is defined as WeakPtr<GLContext> mGL.

Comment 2

[Sotaro Ikeda [:sotaro]]

And there could be a case that GLContext::MarkDestroyed() is called by WebGLContext.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GLContextEGL::OnMarkDestroyed()

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Blocks a bug needing uplift to 65, so tracking this accordingly.

Comment 5

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: It is not easy. Thre is no specific STR to hit the problem.

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?: firefox66

If not all supported branches, which bug introduced the flaw?: Bug 1493497

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: It is easy to back port. It is not risky.

How likely is this patch to cause regressions; how much testing does it need?: It is not likely to cause the regression. Current auto tests already cover the code. And there is no specific STR to hit the problem.

Comment 6

[Sotaro Ikeda [:sotaro]]

The problem directly affect only to firefox66, but it needs to be uplifted to firefox65 because of Bug 1493497.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

Clearing sec-approval as this is a sec-moderate now. We probably still want this on Beta so please nominate a beta patch.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/36be88b03e11661c75387b773e8df82af9ef68cd
https://hg.mozilla.org/mozilla-central/rev/36be88b03e11

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval.

Comment 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1493497

User impact if declined: When Bug 1493497 is uplifted to beta and this is not uplifted, it cause crash.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: Bug 1493497

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is relatively simple for avoiding nullptr dereference.

String changes made/needed: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9034336 [details] [diff] [review]
patch - Add GLContextEGL::OnMarkDestroyed()

[Triage Comment]
Follow-up fix for bug 1493497. Approved for 65.0b12.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44fe44a6a340