[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #95209|review?                     |review-
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2011-05-27 13:45:26 PST ---
(From update of attachment 95209)
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.

> 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.

> 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.

> 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.

> 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.

> 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.

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

Ditto.

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