[webkit-reviews] review granted: [Bug 31809] [CRASH] Assertion on clicking a link : [Attachment 43794] [PATCH] Same with test and ChangeLog for it.

Tue Dec 1 10:08:26 PST 2009

Darin Adler <darin at apple.com> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 31809: [CRASH] Assertion on clicking a link
https://bugs.webkit.org/show_bug.cgi?id=31809

Attachment 43794: [PATCH] Same with test and ChangeLog for it.
https://bugs.webkit.org/attachment.cgi?id=43794&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The bug calls this a crash. What's the behavior in builds without assertions?

--------

> +<body onload="onload();">
> +<p id="description"></p>
> +<div id="console"></div>
> +<script>
> +description("This tests that an iframe with empty id does not crash on
setting location.");
> +var successfullyParsed = true;
> +</script>
> +</body>

This test could be done in the script-tests style, since all the testing is
done with JavaScript code. Next time you do a test like this one I'd request
you do that. The test is a .js file in a script-tests directory and the HTML
wrapper is made by running the make-script-test-wrappers. It's a little more
future-proof to do it that way -- we might change the wrapper in the future.

>      if (inDocument())
> -	   openURL();
> +	   setNameAndOpenURL();

One question here is whether re-executing the logic to set the name is helpful
each time you navigate. A side effect of this patch is that a frame with no
explicit name will get a new name when we navigate. That is not necessarily
something we want. A more-conservative patch would be one that only calls the
name setting logic when m_frameName is empty. This would be less likely to
change behavior in cases that are already working.

But really either behavior would be OK if we had tests to prove it is OK and
compatible with other browsers.

--------

Looking at this patch made me realize there are other minor problems and
inconsistencies with frame names in this class (not part of this bug, but I
thought worth mentioning).

As the FIXMEs in the source code indicate, we don't correctly rename frames
when the attributes change after the frame is loaded. We should at least have
tests for this to see how our behavior relates to the behavior of other
browsers.

But even before loading the frame, if a website changes the id or name
attribute by removing it entirely or by setting it to the empty string,
HTMLFrameElementBase::parseMappedAttribute leaves m_frameName set to the empty
string, which is inconsistent with what setNameAndOpenURL does and probably
wrong.

Further, if before loading the frame a website sets the id attribute on a frame
that already has a name attribute, m_frameName gets the id, overriding the name
from the name attribute. But the rule in the setNameAndOpenURL function is that
if you have both a name and an id, the name wins. So this is another
inconsistency in the code.

Looking at the HTML5 specification, there is no mention of the id attribute
contributing to the frame name for an <iframe> or <frame> element; it's
possible we could remove the code relating to the id attribute completely,
although we would have to test to see that we match other browsers and also try
to figure out if there is any WebKit-specific dependency on this behavior
somehow as well.

I see other interesting inconsistencies with what HTML5 specifies as well, for
example we don't check that the name is a "valid browsing context name" and
thus allow any characters in a frame name.

In these various cases we would like to eventually either have our behavior
match the HTML5 specification or, if we can't change behavior for some reason,
take some action. This could range from documenting our differences and
creating tests showing them up to suggesting a change to HTML5. In particular,
if the behavior with the id attribute here contributing to the frame name is
something shared with other browsers, then I think we want to see that
reflected in the HTML5 specification.

--------

Generally speaking we might be better off if this class did not make an attempt
to store the frame name. Since only thing we ever do with m_frameName is to
pass it to FrameLoader::requestFrame, which passes it to
FrameLoader::loadSubframe, which in turn passes it to
FrameLoaderClient::createFrame. The name is never used when navigating the
existing frame. The only time it's used is when creating a brand new frame. It
seems to me that we could remove the frame name arguments from both the
requestFrame and loadSubframe functions, and add a virtual function to
HTMLFrameOwnerElement called initialFrameName or initialSubframeName. That
function could have the logic that's currently in setNameAndOpenURL -- in the
base class it would just get the name attribute, and in HTMLFrameElementBase it
would be overridden to also consider the ID attribute -- and would be called
only by FrameLoader::loadSubframe. Further, I think the call to the
uniqueChildName function would be better placed in FrameLoader::loadSubframe,
not in the HTMLFrameOwnerElement functions at all.

I'd love to see a patch that does this -- I think it would be a step in the
right direction for the division of responsibilities between the frame loader
and the elements that can trigger loading of subframes.

--------

I think it's probably OK to land this patch as-is.

I'm going to say r=me, but please consider at least some of the suggestions
above.