[webkit-reviews] review granted: [Bug 92774] Crash in FrameLoader::checkLoadComplete with non-browser client app : [Attachment 155591] speculative fix

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 92774: Crash in FrameLoader::checkLoadComplete with non-browser client app
https://bugs.webkit.org/show_bug.cgi?id=92774

Attachment 155591: speculative fix
https://bugs.webkit.org/attachment.cgi?id=155591&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155591&action=review


> Source/WebCore/css/CSSFontSelector.cpp:581
> +    // It is possible load calls could cause CSSFontSelector to get deleted
synchronously.

"CSSFontSelector can be deleted via beginLoadIfNeeded() or loadDone() unless
protected." (or better something more specific about how that could happen).

Both "possible" and "could" make this difficult to read.

> Source/WebCore/css/CSSFontSelector.cpp:582
> +    // If detached clearDocument() will get called ensuring m_document is
null.

I think that you're explaining why m_document won't be a dangling pointer, but
that seems excessive. We don't make such comment every time we use a member
variable pointing to some other object, and don't make it for every variable
that has a clearXXX() method. Is there something unusual here that needs to be
explained?