[webkit-reviews] review requested: [Bug 23626] Upstream null checks in Navigator.cpp : [Attachment 27418] patch v1

[bugzilla-daemon at webkit.org]

Feng Qian <feng at chromium.org> has asked Alexey Proskuryakov <ap at webkit.org> for
review:
Bug 23626: Upstream null checks in Navigator.cpp
https://bugs.webkit.org/show_bug.cgi?id=23626

Attachment 27418: patch v1
https://bugs.webkit.org/attachment.cgi?id=27418&action=review

------- Additional Comments from Feng Qian <feng at chromium.org>
Converted the test into a layout test with results attached.

Two checks in plugins() and mimeTypes() are not needed because both PluginArray
and MimeTypeArray can handle null frame pointer, (but I think it is better to
have null checks so it returns JS null, rather than empty PluginArray and
MimeTypeArray).

The patch triggers an assertion in debug mode:

String WebFrameLoaderClient::userAgent(const KURL& url)
{
    WebView *webView = getWebView(m_webFrame.get());
    // ASSERT(webView);  <----------- HERE 

    // We should never get here with nil for the WebView unless there is a bug
somewhere else.
    // But if we do, it's better to return the empty string than just crashing
on the spot.
    // Most other call sites are tolerant of nil because of Objective-C
behavior, but this one
    // is not because the return value of _userAgentForURL is a const KURL&.
    if (!webView)
	return String("");

    return [webView _userAgentForURL:url];
}

>From comments, someone already encountered such case. I don't know enough why
this happens. So with this patch, release mode will pass, but debug mode will
hit the ASSERT(webView).

Can someone point out what triggers ASSERTION, and whether it is ok to turn off
ASSERT in the debug mode, maybe just a logging message.