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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Feng Qian <feng at chromium.org>'s
request 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 Alexey Proskuryakov <ap at webkit.org>
> (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!