Bug 31809 – Assertion failure in HTMLFrameElementBase::openURL on clicking a link

• Component: WebCore Misc.
• Status: RESOLVED WORKSFORME

[Pavel Feldman]

Open http://user.qzone.qq.com/775036573 page,
Click second link to the right from the floating hearts.
Enjoy the crash:

#0	0x0468b8ba in WebCore::HTMLFrameElementBase::openURL at HTMLFrameElementBase.cpp:93
#1	0x0468ba74 in WebCore::HTMLFrameElementBase::setLocation at HTMLFrameElementBase.cpp:214
#2	0x0468bae0 in WebCore::HTMLFrameElementBase::parseMappedAttribute at HTMLFrameElementBase.cpp:113
#3	0x0468f775 in WebCore::HTMLIFrameElement::parseMappedAttribute at HTMLIFrameElement.cpp:92
#4	0x04c32db0 in WebCore::StyledElement::attributeChanged at StyledElement.cpp:190
#5	0x04a25754 in WebCore::NamedNodeMap::addAttribute at NamedAttrMap.cpp:278
#6	0x045a505e in WebCore::Element::setAttribute at Element.cpp:555
#7	0x045a51da in WebCore::Element::setAttribute at Element.cpp:137
#8	0x04888f9b in WebCore::JSHTMLIFrameElement::setSrc at JSHTMLIFrameElementCustom.cpp:56
#9	0x048877e2 in WebCore::setJSHTMLIFrameElementSrc at JSHTMLIFrameElement.cpp:327
#10	0x04888791 in JSC::lookupPut<WebCore::JSHTMLIFrameElement> at Lookup.h:303
#11	0x048887d6 in JSC::lookupPut<WebCore::JSHTMLIFrameElement, WebCore::JSHTMLElement> at Lookup.h:317
#12	0x04887829 in WebCore::JSHTMLIFrameElement::put at JSHTMLIFrameElement.cpp:274
#13	0x006e7c6a in JSC::JSValue::put at JSObject.h:656

[Pavel Feldman]

Created attachment 43721 [details]
[PATCH] Initial patch to get the things going.

[Adam Barth]

Comment on attachment 43721 [details]
[PATCH] Initial patch to get the things going.

We'd like a test for this.

[Pavel Feldman]

(In reply to comment #2)
> (From update of attachment 43721 [details])
> We'd like a test for this.

I would appreciate if you did a review and request for a test after. I am not sure what I do here is right, that should be clear from the patch name. Now that you r- it, people are not looking at it while I am doing a test. Not helpful.

[Adam Barth]

Sorry, I didn't mean to step on your toes.  Feel free to mark the patch for review again.  You might also want to add some folks to the CC list who are in the svn blame for that function.

[Pavel Feldman]

Created attachment 43724 [details]
[PATCH] Same with test.

[Eric Seidel (no email)]

Personally I find tests essential to understanding most patches. :)  So I would have likely done exactly the same as Adam and r-'d this w/o a test.

[Pavel Feldman]

(In reply to comment #6)
> Personally I find tests essential to understanding most patches. :)  So I would
> have likely done exactly the same as Adam and r-'d this w/o a test.

Ok, ok. I was just upset that after r- people would not be looking at it while I was baking the test. I was sure that the code authors (cc-ed) would find this problem trivial. The patch I provided was a wild guess, I wanted to get feedback early to be able to produce a good fix.

Let me provide a broader context:
- This is one of the top crashers in chromium land according to the dev channel stats
- This is a WebKit bug, hence will affect all WebKit-based browsers
- There is a clear, repeatable way of reproducing it, mentioned in the bug.

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > Personally I find tests essential to understanding most patches. :)  So I would
> > have likely done exactly the same as Adam and r-'d this w/o a test.
> 
> Ok, ok. I was just upset that after r- people would not be looking at it while
> I was baking the test. I was sure that the code authors (cc-ed) would find this
> problem trivial. The patch I provided was a wild guess, I wanted to get
> feedback early to be able to produce a good fix.
> 
> Let me provide a broader context:
> - This is one of the top crashers in chromium land according to the dev channel
> stats
> - This is a WebKit bug, hence will affect all WebKit-based browsers
> - There is a clear, repeatable way of reproducing it, mentioned in the bug.

... and I encouraged him to post the fix right away, hoping there would be feedback. I assumed peeps will see that this is a crasher and treat it accordingly.

BTW, Can haz review please? I am not extremely familiar with these parts of the code.

[Eric Seidel (no email)]

I expect that just as you said, that this will make total sense to someone familiar with this code.  :)  I'm not familiar with this code, but that fix does look sane to me.  Have you grepped for other uses of openURL(), It's not clear to me when we would ever want to call openURL instead of setNameAndOpenURL().

[Eric Seidel (no email)]

Does this test have a testable side-effect?  Should iframe.id be changed after the iframe.src = setting?

[Pavel Feldman]

(In reply to comment #9)
> Have you grepped for other uses of openURL(), It's not
> clear to me when we would ever want to call openURL instead of
> setNameAndOpenURL().

openURL is private and is only called explicitly from setNameAndOpenURL _and_ setLocation.

> Does this test have a testable side-effect?  Should iframe.id be
>> changed after the iframe.src = setting?

I'll check. Not sure if I should set any strict expectations though.

[Pavel Feldman]

> > Does this test have a testable side-effect?  Should iframe.id be
> >> changed after the iframe.src = setting?
> 
> I'll check. Not sure if I should set any strict expectations though.

It stays empty. I can update test with that check, but I am really not sure if that is a reference behavior we should test against.

[Pavel Feldman]

Created attachment 43794 [details]
[PATCH] Same with test and ChangeLog for it.

[Eric Seidel (no email)]

setNameAndOpenURL came from http://trac.webkit.org/changeset/24396

[Adam Barth]

style-queue successfully ran check-webkit-style on attachment 43794 [details] without any errors

[Darin Adler]

Comment on attachment 43794 [details]
[PATCH] Same with test and ChangeLog for it.

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.

[Eric Seidel (no email)]

Attachment 43794 [details] was posted by a committer and has review+, assigning to Pavel Feldman for commit.

[Eric Seidel (no email)]

This patch was posted over 2 months ago, and has been approved for a month.  Are we going to land this?

[WebKit Commit Bot]

Comment on attachment 43794 [details]
[PATCH] Same with test and ChangeLog for it.

Rejecting patch 43794 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12090 test cases.
fast/frames/iframe-set-location-empty-id.html -> failed

Exiting early after 1 failures. 6969 tests run.
118.75s total testing time

6968 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/231617

[Pavel Feldman]

Comment on attachment 43794 [details]
[PATCH] Same with test and ChangeLog for it.

clearing r+ since the tests failed on bots.

[Adam Barth]

We no longer crash here.