[webkit-reviews] review denied: [Bug 61494] REGRESSION (r78342) - Crashes when Document calls into a freed DocumentLoader : [Attachment 95209] Patch v1 - Speculative fix that prevents Document from maintaining a garbage DocumentLoader pointer

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 61494: REGRESSION (r78342) - Crashes when Document calls into a freed
DocumentLoader
https://bugs.webkit.org/show_bug.cgi?id=61494

Attachment 95209: Patch v1 - Speculative fix that prevents Document from
maintaining a garbage DocumentLoader pointer
https://bugs.webkit.org/attachment.cgi?id=95209&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.