Bug 47432 – No longer ASSERT for LayerRenderer in VideoLayerChromium destructor

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

No longer ASSERT for LayerRenderer in VideoLayerChromium destructor

[Victoria Kirst]

Created attachment 70329 [details]
Patch

[Victoria Kirst]

Turns out, we can't guarantee that VideoLayerChromiums will not be created with LayerRenderers associated with them, so we need to change the ASSERT to an if after all. 

In particular, this case can occur if hardware acceleration is supported and initializes correctly, the WebMediaPlayerClientImpl loads correctly, but there is a problem in the media pipeline (such as in demuxing) so the video object is created but never rendered.

[Victoria Kirst]

(In reply to comment #2)
> Turns out, we can't guarantee that VideoLayerChromiums will not be created with LayerRenderers associated with them

Oops, I meant that we can't guarantee VideoLayerChromiums *will* be created with LayerRenderers associated with them! In other words, there will be non-mistake cases where VideoLayerChromiums are destructed without ever having a LayerRenderer attached to them.

[Adam Barth]

Comment on attachment 70329 [details]
Patch

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

Can we have an test that hits this assert (even periodically)?

> WebCore/ChangeLog:17
> -        
> +

This change is spurious and will cause the automatic commit to fail.

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:191
> +    if (layerRenderer()) {

Prefer early return.

[Victoria Kirst]

(In reply to comment #4)
> (From update of attachment 70329 [details])
> 
> Can we have an test that hits this assert (even periodically)?

I'm not sure how to do this in a layout test. What I would need to write is some sort of JavaScript to force WebKit to destruct a WebMediaPlayer... In a UI test this would just involve forcing the browser to do a refresh, but in JS "window.location.reload(true);" is not the same. Any ideas? I can ask my teammates as well.

[Hin-Chung Lam]

There are already layout tests that inject a video element and then remove it from the DOM tree. However test_shell or DRT for chrome doesn't support accelerated compositing, so this bug is not triggered. I think we are working on getting that infrastructure.

[James Robinson]

test_shell and DRT both support accelerated compositing right now (using Mesa).  Pass --accelerated-compositing to new-run-webkit-tests to turn this on.  We're in the process of configuring the bots to run tests in this configuration by default, but in the meantime you should be able to construct a test that triggers the issue when run locally.

I think it's really important to have test coverage for this given how many crashes there have already been in this part of the codebase.

[Victoria Kirst]

(In reply to comment #6)
> There are already layout tests that inject a video element and then remove it from the DOM tree. 

But does this destroy the VideoLayerChromium? When I was playing around with JavaScript on my local build of Chrome (that is, not in test_shell or drt), it seemed like removing the video element from the DOM tree did not destruct the WebMediaPlayer and therefore did not destruct the VideoLayerChromium; only refreshing the page seemed to destroy the layer. I could have done something wrong though. Can you show me which test(s) will actually destroy the VideoLayerChromium? 

On the other hand, if refreshing the page is the only way to destroy the VideoLayerChromium, then I'm still not sure how to write a layout test for this even with Mesa since I'm not sure how to trigger this code path.

[Hin-Chung Lam]

(In reply to comment #8)
> (In reply to comment #6)
> > There are already layout tests that inject a video element and then remove it from the DOM tree. 
> 
> But does this destroy the VideoLayerChromium? When I was playing around with JavaScript on my local build of Chrome (that is, not in test_shell or drt), it seemed like removing the video element from the DOM tree did not destruct the WebMediaPlayer and therefore did not destruct the VideoLayerChromium; only refreshing the page seemed to destroy the layer. I could have done something wrong though. Can you show me which test(s) will actually destroy the VideoLayerChromium? 
> 

Remove a video tag does destroy WebMediaPlayer. I tested this with test_shell.

media/controls-after-reload.html will destroy the WebMediaPlayer. And if --enable-accelerated-compositing is used VideoLayerChromium is also destroyed. So I think it's a different code path that triggers the crash.

> On the other hand, if refreshing the page is the only way to destroy the VideoLayerChromium, then I'm still not sure how to write a layout test for this even with Mesa since I'm not sure how to trigger this code path.

(In reply to comment #8)
> (In reply to comment #6)
> > There are already layout tests that inject a video element and then remove it from the DOM tree. 
> 
> But does this destroy the VideoLayerChromium? When I was playing around with JavaScript on my local build of Chrome (that is, not in test_shell or drt), it seemed like removing the video element from the DOM tree did not destruct the WebMediaPlayer and therefore did not destruct the VideoLayerChromium; only refreshing the page seemed to destroy the layer. I could have done something wrong though. Can you show me which test(s) will actually destroy the VideoLayerChromium? 
> 
> On the other hand, if refreshing the page is the only way to destroy the VideoLayerChromium, then I'm still not sure how to write a layout test for this even with Mesa since I'm not sure how to trigger this code path.

[Hin-Chung Lam]

I tried to create a new layout test for this I can't figure a way to test this without making changes to test_shell.

There are some conditions to trigger this:
1. A video is loaded and the first 2 frames are decoded.
2. Either WebMediaPlayer::repaint is not called or the repaint never reach LayerRendererChromium.

(1) is easy. (2) is hard to do, it is possible that repaint is fired but before it reaches LayerRendererChromium we destroy WebMediaPlayer, this is very time dependent and is not realistic. What is really causing this crash is that LayerRendererChromium failed to be initialized in a valid GL context, so we created a VideoLayerChromium and never associate it with a LayerRendererChromium.

I wonder if we can modify LayoutTestController to allow tests to cripple GL context initialization. Or we add a flag to run test_shell with --fail-mesa-initialization and --enable-accelerated-compositing to make sure tests don't crash.

Besides testing, instead of early return if we have a good way to check if compositor is inactive we can skip creating VideoLayerChromium, any thoughts on this?

[Vangelis Kokkevis]

> 
> Besides testing, instead of early return if we have a good way to check if compositor is inactive we can skip creating VideoLayerChromium, any thoughts on this?

As the code stands right now, the VideoLayerChromium gets created after checking for supportsAcceleratedRendering().  However, supportsAcceleratedRendering() uses the value from contentRenderer()->compositor()->hasAcceleratedCompositing() captured when the WebMediaPlayerClientImpl is first created which isn't necessarily accurate.  One possibility would be to not cache that return value and try to get it every time supportsAcceleratedRendering(). Hopefully this will give a more accurate indication on whether the LayerRendererChromium was successfully created. Even that though isn't guaranteed to be accurate, especially if readyStateChanged() gets called before the compositor for the page is set up.

Overall I think the safest and correct solution is the one currently implemented in the patch.

I think the proper way to test this and other similar failures would be to have a mode in DRT that simulates the failure to create a GL context so that we can test that all the fallback paths work properly.

[Vangelis Kokkevis]

(In reply to comment #11)

> I think the proper way to test this and other similar failures would be to have a mode in DRT that simulates the failure to create a GL context so that we can test that all the fallback paths work properly.

Entered in chromium bug tracker as:
http://code.google.com/p/chromium/issues/detail?id=59059

[Vangelis Kokkevis]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > I think the proper way to test this and other similar failures would be to have a mode in DRT that simulates the failure to create a GL context so that we can test that all the fallback paths work properly.
> 
> Entered in chromium bug tracker as:
> http://code.google.com/p/chromium/issues/detail?id=59059

James, Adam: Any chance we can check this patch in (this is now a top crasher for chromium) and deal with building the testing infrastructure for the software fallback as a separate issue? It falls somewhat outside the scope of this bug.

[Victoria Kirst]

Created attachment 70633 [details]
Patch

[James Robinson]

VideoLayerChromium shutdown has been the top crasher for the last few weeks now and we've been through several iterations of fixes but are still finding issues.  I think it's really high priority to get more test coverage here or we'll keep running in to more issues.

[Victoria Kirst]

Created attachment 70640 [details]
Patch

[Vangelis Kokkevis]

(In reply to comment #15)
> VideoLayerChromium shutdown has been the top crasher for the last few weeks now and we've been through several iterations of fixes but are still finding issues.  I think it's really high priority to get more test coverage here or we'll keep running in to more issues.

Agreed on adding tests for this type of situation.  Assuming that the failure is on devices where we fail to create a GL context I think the proper testing needs to happen at a different level and that's why I believe it's outside the scope of this CL. I don't have any good ideas on how, from JS only, to trigger a failure to create a GL context.

We could delay checking this in until we adjust our testing infrastructure to simulate a GL context creation failure and run through all our tests as I suggested in the chromium bug but I find this to be counter productive. This failure could be masking other video issues which we won't get to find until this particular crash is resolved.

[James Robinson]

Comment on attachment 70640 [details]
Patch

Patch is fine, let's move the discussion about testing to another bug.

[Hin-Chung Lam]

Starting a new bug to discuss how we can make our tools / infrastructure better: https://bugs.webkit.org/show_bug.cgi?id=47625

[Hin-Chung Lam]

Committed as http://trac.webkit.org/changeset/69706.

[Eric Seidel (no email)]

Please close bugs after committing.  webkit-patch land-from-bug will do that for you (as will webkit-patch land).