REGRESSION(r122215) - RenderObject::willRenderImage crashes on null view()
Created attachment 152812 [details] Patch
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?
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?
(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.
Created attachment 152841 [details] Reword comment
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.
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.
Committed r122886: <http://trac.webkit.org/changeset/122886>
(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. :)