Bug 88815 – [chromium] Don't crash when m_private->layerTreeHost()->context() is NULL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ami Fischman]

I think r119313 introduced a crasher; see backtrace below.  Fix seems simple enough: return 0 if !m_private->layerTreeHost()->context().  I'll upload a patch.

(gdb) bt
#0  0x00007f2cb89daa32 in WTF::RefPtr<WebCore::GraphicsContext3D>::get (this=0x20) at ../../third_party/WebKit/Source/WTF/wtf/RefPtr.h:58
#1  0x00007f2cb89da8ea in WebCore::CCGraphicsContext::context3D (this=0x0) at ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:47
#2  0x00007f2cb89da874 in WebKit::WebLayerTreeView::context (this=0x7f2cb496faf8) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebLayerTreeView.cpp:170
#3  0x00007f2cb8a17096 in WebKit::WebViewImpl::graphicsContext3D (this=0x7f2cb496f880) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:3659
#4  0x00007f2cc083065a in RenderViewImpl::createMediaPlayer (this=0x7f2cae18d000, frame=0x7f2cad4d6e00, client=0x7f2cac8a43d0) at ../../content/renderer/render_view_impl.cc:2369
#5  0x00007f2cb89db5bb in WebKit::createWebMediaPlayer (client=0x7f2cac8a43d0, frame=0x7f2caccb6000) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:56
#6  0x00007f2cb89dc77f in WebKit::WebMediaPlayerClientImpl::loadInternal (this=0x7f2cac8a43c0) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:314
#7  0x00007f2cb89dc6ad in WebKit::WebMediaPlayerClientImpl::load (this=0x7f2cac8a43c0, url=...) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:303
#8  0x00007f2cb975d18f in WebCore::MediaPlayer::loadWithNextMediaEngine(WebCore::MediaPlayerFactory*) () from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#9  0x00007f2cb975cea0 in WebCore::MediaPlayer::load(WebCore::KURL const&, WebCore::ContentType const&, WTF::String const&) () from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#10 0x00007f2cba03db3e in WebCore::HTMLMediaElement::loadResource(WebCore::KURL const&, WebCore::ContentType&, WTF::String const&) ()
   from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#11 0x00007f2cba03d45d in WebCore::HTMLMediaElement::selectMediaResource() () from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#12 0x00007f2cba03d135 in WebCore::HTMLMediaElement::loadInternal() () from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#13 0x00007f2cba03cc15 in WebCore::HTMLMediaElement::load(int&) () from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#14 0x00007f2cba308522 in WebCore::HTMLMediaElementV8Internal::loadCallback(v8::Arguments const&) () from /home/fischman/chrome_root/src/out_lumpy/Debug/lib/libwebkit.so
#15 0x00007f2cbef531b3 in v8::internal::HandleApiCallHelper<false> (args=..., isolate=0x7f2cb48b9000) at ../../v8/src/builtins.cc:1145
#16 0x00007f2cbef4d54c in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=0x7f2cb48b9000) at ../../v8/src/builtins.cc:1162
#17 0x00007f2cbef4d51d in v8::internal::Builtin_HandleApiCall (args=..., isolate=0x7f2cb48b9000) at ../../v8/src/builtins.cc:1161

[Ami Fischman]

Created attachment 146934 [details]
Patch

[James Robinson]

Comment on attachment 146934 [details]
Patch

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

Test?  In what situation is ->context() NULL?

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:171
> +    WebCore::CCGraphicsContext* c = m_private->layerTreeHost()->context();
> +    if (!c)

we don't use one-letter variable names in WebKit (probably because we don't have an 80 column limit), please give this a better name.  Perhaps "context" ?

[Ami Fischman]

Created attachment 146939 [details]
Patch

[Ami Fischman]

(In reply to comment #2)
> (From update of attachment 146934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146934&action=review
> 
> Test?  In what situation is ->context() NULL?

I don't know.  I was hoping the backtrace in the bug report would be enough for you to divine what's gone wrong.

> > Source/WebKit/chromium/src/WebLayerTreeView.cpp:171
> > +    WebCore::CCGraphicsContext* c = m_private->layerTreeHost()->context();
> > +    if (!c)
> 
> we don't use one-letter variable names in WebKit (probably because we don't have an 80 column limit), please give this a better name.  Perhaps "context" ?

I dispute that "context" is any clearer in this context, but done ;)

[James Robinson]

Comment on attachment 146939 [details]
Patch

Looking more closely it seems quite reasonable that we would get here and extractWeb...() handles a null parameter fine, but we need a test to avoid regressing this (if we had one then we wouldn't have broken this in r119313)

[Ami Fischman]

(In reply to comment #5)
> (From update of attachment 146939 [details])
> Looking more closely it seems quite reasonable that we would get here and extractWeb...() handles a null parameter fine, but we need a test to avoid regressing this (if we had one then we wouldn't have broken this in r119313)

Since you understand this stuff 10000x better than me can you suggest a useful test scenario that'd hit this case?

(obvs I could construct a mock something to tickle the codepath, but that doesn't seem very useful)

[Ami Fischman]

James/Alexandre: ping?  Can you make a concrete suggestion for testing (comment #6), or r+ this without test?  (I think we all agree this is a uniformly-positive change to the codebase, what with the not crashing and all).

[James Robinson]

With the fix for http://code.google.com/p/chromium/issues/detail?id=129103 is this still relevant?

[Ami Fischman]

(In reply to comment #8)
> With the fix for http://code.google.com/p/chromium/issues/detail?id=129103 is this still relevant?

Reverting the proposed patch locally, I no longer see the crashes (which were intermittent before).
Are you comfortable asserting m_private->layerTreeHost()->context() is never NULL?

[James Robinson]

When that patch lands I'm more comfortable deleting the getter that lets you hit this codepath :)

[Ami Fischman]

(In reply to comment #10)
> When that patch lands I'm more comfortable deleting the getter that lets you hit this codepath :)

SGTM!

[James Robinson]

I've got patches on the way to remove WebView::graphicsContext3D() which I think will make this crash impossible.