| Summary: | REGRESSION(r124168): Null crash in RenderLayer::createScrollbar | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Abhishek Arya <inferno> | ||||||||
| Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | enne, eric, jchaffraix, simon.fraser, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Abhishek Arya
2012-09-15 09:23:11 PDT
From regression range, this looks to regress in https://trac.webkit.org/changeset/124168/. Julien, is this the same null scrollbar crash you were mentioning a few days back. (In reply to comment #1) > From regression range, this looks to regress in https://trac.webkit.org/changeset/124168/. It's definitely a regression from this change. > Julien, is this the same null scrollbar crash you were mentioning a few days back. Nope, the other crasher was bug 93903. This is another regression. Created attachment 165499 [details]
Test patch for an early EWS run.
Created attachment 165688 [details]
Proposed fix: avoid triggering a style change if we only need a temporary style.
Comment on attachment 165688 [details] Proposed fix: avoid triggering a style change if we only need a temporary style. View in context: https://bugs.webkit.org/attachment.cgi?id=165688&action=review > Source/WebCore/rendering/RenderObject.cpp:135 > + // RenderImageResource requires a style being present on the image but we don't want to typo s/RenderImageResource/RenderImageResourceStyleImage. i actually debugged this to understand what is going on and can see the addClient call invoked through RenderImageResourceStyleImage::initialize. Also please add a comment on why this code with the "if (contentData && !contentData->next() && contentData->isImage() && doc != node)" not be moved to RenderImage::styleDidChange ? > LayoutTests/scrollbars/scrollbar-content-crash.html:12 > +function passed() { 'passed' sounds weird since the test might fail. How about something like runTest. Comment on attachment 165688 [details] Proposed fix: avoid triggering a style change if we only need a temporary style. Actually I found a more cleaner approach and i think the bug is in RenderImage::imageChanged called by CachedImage::didAddClient. We should just bail out if we don't have a parent. See similar [http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp&exact_package=chromium&q=RenderBox::imageChanged%20file:webcore&l=989] So, the fix should be to get rid of setStyle which is in the wrong place here. And bail out on no parent in imageChanged, since it is later called when the real setAnimatableStyle is called. RenderObject* NodeRendererFactory::createRenderer() { Node* node = m_context.node(); RenderObject* newRenderer = node->createRenderer(node->document()->renderArena(), m_context.style()); if (!newRenderer) ..... node->setRenderer(newRenderer); newRenderer->setAnimatableStyle(m_context.releaseStyle()); > So, the fix should be to get rid of setStyle which is in the wrong place here. And bail out on no parent in imageChanged, since it is later called when the real setAnimatableStyle is called.
It's OK for most RenderObject to bail out of imageChanged if you don't have a parent as they only trigger a repaint (which would be a no-op anyway). RenderImage is not among them as it uses the signal to recompute its intrinsic size.
AFAICT there is no guarantee imageChanged will be called asynchronously after you have been inserted into the tree so this approach makes at least 2 tests flaky (fast/css/contentDiv.html and fast/css/contentImage.html).
RenderObjectChildList has a similar code to handle image content and also calls setStyle before calling setImageResource which makes me believe it's needed. What's not needed is triggering a style change notification in RenderObject::createObject.
Created attachment 166321 [details]
Patch for landing
Comment on attachment 166321 [details] Patch for landing Clearing flags on attachment: 166321 Committed r129955: <http://trac.webkit.org/changeset/129955> All reviewed patches have been landed. Closing bug. |