Bug 91525 – REGRESSION(r122215) - RenderObject::willRenderImage crashes on null view()

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

[Adrienne Walker]

REGRESSION(r122215) - RenderObject::willRenderImage crashes on null view()

[Adrienne Walker]

Created attachment 152812 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 152812 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:2697
> +    if (!view())

So this is RenderView...  And document()->view() is FrameView?

[Eric Seidel (no email)]

Comment on attachment 152812 [details]
Patch

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

>> Source/WebCore/rendering/RenderObject.cpp:2697
>> +    if (!view())
> 
> So this is RenderView...  And document()->view() is FrameView?

Yes, it is.  And we should really make that less confusing some day.

I'm confused what callstack would hit this?  I guess document destruction?  IT's not clear what you mean by "not been inserted into the tree", do you mean the document or the RenderImage?

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 152812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152812&action=review
> 
> >> Source/WebCore/rendering/RenderObject.cpp:2697
> >> +    if (!view())
> > 
> > So this is RenderView...  And document()->view() is FrameView?
> 
> Yes, it is.  And we should really make that less confusing some day.
> 
> I'm confused what callstack would hit this?  I guess document destruction?  IT's not clear what you mean by "not been inserted into the tree", do you mean the document or the RenderImage?

Maybe I shouldn't have included any comment at all.  ;)

Document destruction is one case, since the Document's renderer is cleared during that step.  Document attachment is the other, since that's when the renderer gets initially set.

I'm guessing at this being the issue given the callstack in the crashreport in the linked Chromium bug.  I wasn't able to repro this locally.

[Adrienne Walker]

Created attachment 152841 [details]
Reword comment

[Kwang Yul Seo]

Comment on attachment 152841 [details]
Reword comment

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

> Source/WebCore/rendering/RenderObject.cpp:2699
> +

The patch looks good to me. I'm sorry to miss this in r122215.

[Julien Chaffraix]

Comment on attachment 152841 [details]
Reword comment

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

> Source/WebCore/ChangeLog:10
> +

It would be nice to mention why you couldn't produce a test case in your ChangeLog.

> Source/WebCore/rendering/RenderObject.cpp:2696
> +    // If the document is being destroyed or has not been attached, then this
> +    // RenderObject will not be rendered.

I am fine with the comment as-is though Eric may want to comment further.

[Adrienne Walker]

Committed r122886: <http://trac.webkit.org/changeset/122886>

[Adrienne Walker]

(In reply to comment #7)
> (From update of attachment 152841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152841&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +
> 
> It would be nice to mention why you couldn't produce a test case in your ChangeLog.

Done.

> > Source/WebCore/rendering/RenderObject.cpp:2696
> > +    // If the document is being destroyed or has not been attached, then this
> > +    // RenderObject will not be rendered.
> 
> I am fine with the comment as-is though Eric may want to comment further.

Sorry for going ahead and landing this, but I wanted to have a chance to get this in tomorrow's canary and see if it reduced the crash rate.  I'm happy to change the comment in a follow-up patch if you think that can be reworded better.  :)