Bug 47020 – [chromium] Fixing crash when audio media player is destructed

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

Fixing crash when audio media player is destructed

[Victoria Kirst]

Created attachment 69521 [details]
Patch

[James Robinson]

Comment on attachment 69521 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69521&action=review

r- for mangled ChangeLog.  Any chance of a test?  When will the layerRenderer() test fail?

> WebCore/ChangeLog:22
> [phone]  Victoria Kirst  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Makes check in destructor to see if renderer was hooked up to the
> +        layer before destructing object.
> +
> +        * platform/graphics/chromium/VideoLayerChromium.cpp:
> +        (WebCore::VideoLayerChromium::~VideoLayerChromium):
> +
> [phone]  Victoria Kirst  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixing crash when audio media player is destructed
> +        https://bugs.webkit.org/show_bug.cgi?id=47020
> +
> +        No new tests. (OOPS!)
> +
> +        * platform/graphics/chromium/VideoLayerChromium.cpp:
> +        (WebCore::VideoLayerChromium::~VideoLayerChromium):
> +

Looks like you ran prepare-ChangeLogs twice.  Please resolve.

[Hin-Chung Lam]

This probably needs to be a chrome ui test since the compositor is not active in test_shell. Victoria: You can write a test in media_uitest.cc that loads an audio element and then switch to other page. A even better one would be a "in browser" ui test where you start a new tab with an audio element and then close the tab.

[Victoria Kirst]

(In reply to comment #2)

> r- for mangled ChangeLog. 
Oops! Thanks for the catch.

> Any chance of a test?  When will the layerRenderer() test fail?
So, it's a little tricky: LayerChromiums do not get initially created with a LayerRenderer associated with it, but instead there is a setLayerRenderer call which creates the association. Thus VideoLayerChromium only gets a LayerRenderer associated with it when the LayerRenderer is drawing that video layer, since that is where the LayerRenderer calls setLayerRenderer() (https://cs.corp.google.com/p#chrome/trunk/src/third_party/WebKit/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp&q=setLayerRenderer&exact_package=chrome&l=473).

In other words, the layerRenderer() test fails if we have created a VideoLayerChromium that is never drawn by the LayerRenderer... That in itself is kind of hard to test, but I can try to write a test that makes sure Audio and Video tags are destructed correctly (which was the problem that was causing all the crashes).

[Victoria Kirst]

Created attachment 69530 [details]
Patch

[Victoria Kirst]

This new patch just fixes the ChangeLogs. I am going to do like Alpha suggested and write UI tests for destructing audio tags and I'll submit those in a separate chromium patch. Does that sound ok, James?

[James Robinson]

Comment on attachment 69530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69530&action=review

Is the change in WebMediaPlayerClientImpl.cpp addressing the same bug, or something different?  I don't understand how that is related to the crash.

> WebCore/ChangeLog:9
> +        Makes check in destructor to see if renderer was hooked up to the
> +        layer before destructing object.
> +
> +        Fixing crash when audio media player is destructed
> +        https://bugs.webkit.org/show_bug.cgi?id=47020

nit: should be one line description, bug link, newline, long description

> WebKit/chromium/ChangeLog:9
> +        Creates the VideoLayerChromium layer only if the media player contains
> +        a video.
> +
> +        Fixing crash when audio media player is destructed
> +        https://bugs.webkit.org/show_bug.cgi?id=47020

nit: should be one line description, bug link, newline, long description

[Victoria Kirst]

Created attachment 69533 [details]
Patch

[Victoria Kirst]

(In reply to comment #7)
> Is the change in WebMediaPlayerClientImpl.cpp addressing the same bug, or something different?  I don't understand how that is related to the crash.

Yes, it is addressing the same bug. The WebMediaPlayerClientImpl object is created for all media players, including audio. The crash was occurring because the VideoLayerChromium platform layer was being constructed in the WebMediaPlayerClientImpl::create() method, which meant the (unused) VLC layer was being created for audio players. When destructing the audio players, the VideoLayerChromiums associated with them were never rendered and therefore did not have a LayerRenderer associated with it, which caused the layerRenderer() assert to fail.

So actually, the crash could be fixed with the WebMediaPlayerClientImpl.cpp change alone, but it makes sense to also add the if (!layerRenderer()) ... check into VideoLayerChromium.cpp for extra security.

> nit: should be one line description, bug link, newline, long description
Fixed x 2!

[James Robinson]

Comment on attachment 69533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69533&action=review

R=me

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:190
> +    if (!layerRenderer())
> +        return;
> +

If we expect this never to be the case, it's probably better to add an ASSERT() instead and watch for issues in the field.  We don't want to paper over real bugs if we don't understand how they might be happening.

[Victoria Kirst]

(In reply to comment #10)
> If we expect this never to be the case, it's probably better to add an ASSERT() instead and watch for issues in the field.  We don't want to paper over real bugs 
if we don't understand how they might be happening.

Updated. Thanks for the review, James! Alpha committed this patch for me here:
http://trac.webkit.org/changeset/69022