Summary: | [chromium] Don't crash when m_private->layerTreeHost()->context() is NULL | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ami Fischman <fischman> | ||||||
Component: | New Bugs | Assignee: | Ami Fischman <fischman> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aelias, fischman, jamesr | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ami Fischman
2012-06-11 15:39:22 PDT
Created attachment 146934 [details]
Patch
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" ? Created attachment 146939 [details]
Patch
(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 ;) 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) (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) 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). With the fix for http://code.google.com/p/chromium/issues/detail?id=129103 is this still relevant? (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? When that patch lands I'm more comfortable deleting the getter that lets you hit this codepath :) (In reply to comment #10) > When that patch lands I'm more comfortable deleting the getter that lets you hit this codepath :) SGTM! I've got patches on the way to remove WebView::graphicsContext3D() which I think will make this crash impossible. |