[Webkit-unassigned] [Bug 61494] REGRESSION (r78342) - Crashes when Document calls into a freed DocumentLoader

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61494





--- Comment #8 from Brady Eidson <beidson at apple.com>  2011-05-27 13:59:39 PST ---
(In reply to comment #7)
> (From update of attachment 95209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95209&action=review
> 
> review- because of the issue inside Document::loader checking the frame’s document.
> 
> > Source/WebCore/bindings/ScriptControllerBase.cpp:-119
> > +        // We're still in a frame, so there should be a DocumentLoader.
> > +        ASSERT(m_frame->document()->loader());
> >          m_frame->document()->loader()->writer()->replaceDocument(scriptResult);
> > -
> 
> Seems safer to have a null check after the assert. We often use that idiom in cases where there is a bit of complexity and uncertainty.

Done.

> 
> > Source/WebCore/dom/Document.cpp:1110
> > -    return m_documentLoader->responseMIMEType();
> > +    if (DocumentLoader* documentLoader = loader())
> > +        return documentLoader->responseMIMEType();
> > +    return String();
> 
> I like early return better than this idiom, but either is OK.

In the context of this method, one could argue this is the early return idiom, or one could argue the other way.  I'm leaving it as-is.

> 
> > Source/WebCore/dom/Document.cpp:3787
> > -        String httpLastModified = m_documentLoader->response().httpHeaderField("Last-Modified");
> > +        // Since we're still in a Frame, we should have a DocumentLoader.
> > +        ASSERT(loader());
> > +        String httpLastModified = loader()->response().httpHeaderField("Last-Modified");
> 
> Even with the assertion, I suggest considering a null check too.

Done.

> 
> > Source/WebCore/dom/Document.cpp:4520
> > +        // Since we're still in a Frame, we should have a DocumentLoader.
> > +        ASSERT(loader());
> > +        if (loader()->substituteData().isValid())
> >              securityOrigin()->grantLoadLocalResources();
> 
> Even with the assertion, I suggest having a null check too.

Done.

> 
> > Source/WebCore/dom/Document.cpp:5164
> > +    ASSERT(m_frame->document() == this);
> > +    
> > +    return loader;
> 
> I think this assertion should be a runtime check and we should return 0 if the frame has moved on to another document.
> 
> It’s a valid state for a document to still be associated with a frame and the frame to have moved on to a new document. In that case, the document loader on the frame is not associated with this document, and so this function should return 0.

Good point, changed.

> 
> > Source/WebCore/html/MediaDocument.cpp:225
> > +        ASSERT(loader());
> >          embedElement->setAttribute(typeAttr, loader()->writer()->mimeType());
> 
> I’m not sure it’s valuable to assert something is non-null just before dereferencing it. We may be going too extreme with the assertions here.

I changed it to an ASSERT + null-check combo, as before.  I'd like to leave the ASSERT while we're figuring out what our long term solution for this code is.

> > Source/WebCore/html/PluginDocument.cpp:96
> > +    ASSERT(document()->loader());
> >      m_embedElement->setAttribute(typeAttr, document()->loader()->writer()->mimeType());
> 
> Ditto.

Ditto for me, too!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.