Bug 23626 – Upstream null checks in Navigator.cpp

• Component: DOM
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Upstream null checks in Navigator.cpp

From here:
http://build.chromium.org/merge/WebCore-page-Navigator.cpp-before.diff

According to Feng, these are needed to protect against some crashes.

[Alexey Proskuryakov]

I'd be interested to learn what crashes those were (and of course, automated test cases would be good).

[Feng Qian]

Created attachment 27405 [details]
a test case (not layout test yet)

I changed Product from WebKit to Security for caution. The test case can crash the browser in various places.

Making a patch.

[Feng Qian]

Created attachment 27418 [details]
patch v1

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.

[Alexey Proskuryakov]

Comment on attachment 27418 [details]
patch v1

> (but I think it is better to have null checks so it returns JS null,
> rather than empty PluginArray and MimeTypeArray).

Sounds reasonable to me. What does Firefox do?

> The patch triggers an assertion in debug mode:

This assertion was added in <http://trac.webkit.org/changeset/32500>. I think that it is useful, because it helps catch attempts to navigate detached frames. I think that a check for frame->page() in Navigator::userAgent() should resolve the assertion failure in a way that doesn't conflict with Darin's intentions (and the check for document can be removed, because we now always have a document in each frame):

-    return m_frame->loader()->userAgent(m_frame->document() ? m_frame->document()->url() : KURL());
+    return m_frame->loader()->userAgent(m_frame->page() ? m_frame->document()->url() : KURL());

Of course, this warrants a comment explaining why the check for page() is necessary.

+  old_nav = window.subframe.navigator;

This makes the test fail in Firefox for no good reason, window.frames[0] works in both browsers.

+  if (window.GCController)
+    window.GCController.collect();

I do not see why this is necessary - but if you prefer to invoke GC, it's better to do it in browser, too (see e.g. fast/workers/worker-gc.html for how we usually do it).

+  // In layout test mode, check one more time later.
+  if (window.layoutTestController)
+    setTimeout(check_navigator_and_done, 200);

That's a good check - why not make it in browser, too? During the first check, the frame is detached from its page, but not destroyed yet - and during the second one, the frame is already destroyed, and thus detached from its Navigator object.

This is obviously an r-, because the test crashes in debug mode due to the assertion in WebFrameClient - looks great otherwise!

[Feng Qian]

(In reply to comment #4)
> (From update of attachment 27418 [details] [review])
> > (but I think it is better to have null checks so it returns JS null,
> > rather than empty PluginArray and MimeTypeArray).
> 
> Sounds reasonable to me. What does Firefox do?
> 
> > The patch triggers an assertion in debug mode:
> 
> This assertion was added in <http://trac.webkit.org/changeset/32500>. I think
> that it is useful, because it helps catch attempts to navigate detached frames.
> I think that a check for frame->page() in Navigator::userAgent() should resolve
> the assertion failure in a way that doesn't conflict with Darin's intentions
> (and the check for document can be removed, because we now always have a
> document in each frame):
> 
> -    return m_frame->loader()->userAgent(m_frame->document() ?
> m_frame->document()->url() : KURL());
> +    return m_frame->loader()->userAgent(m_frame->page() ?
> m_frame->document()->url() : KURL());
> 

This does not solve the problem. Do you mean, if m_frame->page() is null, return an empty string instead of calling into FrameLoader::userAgent? Because FrameLoader::userAgent does not check what URL is passed in.

> Of course, this warrants a comment explaining why the check for page() is
> necessary.
> 
> +  old_nav = window.subframe.navigator;
> 
> This makes the test fail in Firefox for no good reason, window.frames[0] works
> in both browsers.
> 
> +  if (window.GCController)
> +    window.GCController.collect();
> 
> I do not see why this is necessary - but if you prefer to invoke GC, it's
> better to do it in browser, too (see e.g. fast/workers/worker-gc.html for how
> we usually do it).
> 
> +  // In layout test mode, check one more time later.
> +  if (window.layoutTestController)
> +    setTimeout(check_navigator_and_done, 200);
> 
> That's a good check - why not make it in browser, too? During the first check,
> the frame is detached from its page, but not destroyed yet - and during the
> second one, the frame is already destroyed, and thus detached from its
> Navigator object.
> 
> This is obviously an r-, because the test crashes in debug mode due to the
> assertion in WebFrameClient - looks great otherwise!
> 

[Feng Qian]

Created attachment 27493 [details]
patch v2

Corrected the patch by incorporating ap's comments.

1. FF returns a PluginArray/MimeTypeArray object when a frame is detached, so WebKit does not need check here;
2. Test case is fixed by calling the same gc() function, although I don't like it relies on 90000 object allocations;
3. Navigator::userAgent() returns an empty string if m_frame->page() is null, avoid calling FrameLoader::userAgent which triggers assertion later in WebFrameLoaderClient::userAgent

Interestingly FF3.1b2 on Mac crashes when loading the layout test. :(

[Alexey Proskuryakov]

Comment on attachment 27493 [details]
patch v2

> +    // If the frame does not have a page, returns empty string instead of
> +    // calling FrameLoader::userAgent.
+    if (!m_frame->page())
+        return String();
+        
     return m_frame->loader()->userAgent(m_frame->document() ? m_frame->document()->url() : KURL());

This is not a helpful comment - it says an extremely obvious thing about what happens below, but doesn't explain the subtle reason. Also, I still think that the check for document() can be removed.

> +  // In layout test mode, check one more time later.
> +  setTimeout(check_navigator_and_done, 200);

This comment is no longer true, and should be removed.

r=me, but comments will need to be tweaked when landing. Did I already say I love this test case?

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/40814>.