Bug 23310 – Setting an absolute path (/abs) on an <iframe> with no src doesn't resolve the URL properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[David Kilzer (:ddkilzer)]

* SUMMARY
When using JavaScript to set the src attribute of an <iframe> element with no src element declared, the URL created is not resolved properly (e.g., using the parent's URL).

* STEPS TO REPRODUCE
1. Launch WebKit/Safari.
2. Open attached test case.
3. Click on "Click Me" button.

* EXPECTED RESULTS
The root of the web site should be opened.

* ACTUAL RESULTS
Nothing happens.  (No error message in the Activity Window or the Web Inspector's console on Safari.)

* REGRESSION
Unknown.  Only tested on Safari 3.2 and WebKit nightly r39872.

* NOTES
This affects a click-tracking mechanism on <http://www.tv.yahoo.com/falltv/photos/fresh-fall-faces/356> when clicking "Next Photo" or "Previous Photo".

[David Kilzer (:ddkilzer)]

Created attachment 26694 [details]
Test case

[David Kilzer (:ddkilzer)]

Created attachment 26695 [details]
Test case (no JavaScript)

Essentially the same HTML as the original test case (Attachment #26694 [details]), but with the src attribute set to "/" in the <iframe> element.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Created an attachment (id=26694) [review]
> Test case

This works as expected in FIrefox 3.

[Adam Barth]

Strange.  In Firefox 3 javascript:alert(frames[0].document.baseURI) alerts "about:blank" but when they resolve "/" they use a different baseURI.

[David Kilzer (:ddkilzer)]

Alexey, do any of your recent KURL changes affect the test case on this bug?

[Alexey Proskuryakov]

No - as Adam says, this is an issue with what base URL is used, not with parsing.

[Alexey Proskuryakov]

Do these changes or existing text cover this already? <http://html5.org/tools/web-apps-tracker?from=2710&to=2711>

[Adam Barth]

(In reply to comment #7)
> Do these changes or existing text cover this already?
> <http://html5.org/tools/web-apps-tracker?from=2710&to=2711>

Adding Ian to the CC list for his opinion.

The text in the spec might be based on our current implementation (or at least the test cases I used to figure out what our behavior should be).

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Do these changes or existing text cover this already?
> <http://html5.org/tools/web-apps-tracker?from=2710&to=2711>

Post-edited content:
<http://www.whatwg.org/specs/web-apps/current-work/#document-base-url>

[Ian 'Hixie' Hickson]

The spec was based on IE.

The first test case should result in the <iframe>, after clicking, containing https://bugs.webkit.org/.
The second test case should result in an <iframe>, regardless of clicking, containing the same page, though for very different reasons.

[vikram hegde]

Actually i tried a fix which is working fine.I modified the
base url in document.cpp,which gives the input to kurl, the referring url as
"about:blank" which causes the kurl to load an empty page, so once i update
the base url in document to the actual url, the bug is not seen.

Kindly let me know whether this is the correct approach.I have tried regressions also and the fix seems to be working fine.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> Actually i tried a fix which is working fine.I modified the
> base url in document.cpp,which gives the input to kurl, the referring url as
> "about:blank" which causes the kurl to load an empty page, so once i update
> the base url in document to the actual url, the bug is not seen.
> 
> Kindly let me know whether this is the correct approach.I have tried
> regressions also and the fix seems to be working fine.

That sounds like the correct approach.  The next step would be to post a patch with a layout test for review.

[Adam Barth]

(In reply to comment #11)
> Actually i tried a fix which is working fine.I modified the
> base url in document.cpp,which gives the input to kurl,

After you make this change, do we still match Firefox's behavior (described in Comment #4)?

> In Firefox 3 javascript:alert(frames[0].document.baseURI) alerts
> "about:blank" but when they resolve "/" they use a different baseURI.

[vikram hegde]

>After you make this change, do we still match Firefox's behavior (described in
>Comment #4)?

> In Firefox 3 javascript:alert(frames[0].document.baseURI) alerts
> "about:blank" but when they resolve "/" they use a different baseURI.

Could you please elaborate on comment #4, I am not changing the baseuri anywhere, only passing the appopriate values to the KURL depending on the current url.The rest all remins in tact.Could you provide more description on the same.

[Adam Barth]

(In reply to comment #14)
> Could you please elaborate on comment #4,

Go to https://bugs.webkit.org/attachment.cgi?id=26694
Type javascript:alert(frames[0].document.baseURI) in the address bar.
Make sure it shows the same message as in Firefox.

[vikram hegde]

Created attachment 27904 [details]
Screenshots of the behaviour in firefox and safari

With the changes i have made, I tested the behavior of javascript:alert(frames[0].document.baseURI) in both firefox and safari, I have taken the screen shots of the actual behaviour in both cases.Kinldy let me know whether this is the expected behaviour.

[Adam Barth]

(In reply to comment #16)
> Screenshots of the behaviour in firefox and safari

You seem to have clicked the "CLICK ME" button.  Please try the experiment again without clicking "CLICK ME".

[vikram hegde]

Created attachment 27906 [details]
screen shots taken before CLICK ME

The behaviour in firefox and safari before clicking on "click me".Kindly let me know the behaviour is as expected.

[Adam Barth]

(In reply to comment #18)
> screen shots taken before CLICK ME

Those look great, thanks!

[vikram hegde]

Created attachment 27907 [details]
Patch for the bug

The attachment is the patch for the mentioned bug.

[Adam Barth]

Can you upload the patch as a diff instead of as a tar?  That way we can use bugzilla to look at the patch.  Thanks for working on this bug.

[vikram hegde]

Created attachment 27909 [details]
Patch for the bug

Patch for the bug in diff format.

[vikram hegde]

Created attachment 27910 [details]
Patch for the bug

This is the actual patch with all the changes.

[David Kilzer (:ddkilzer)]

Comment on attachment 27910 [details]
Patch for the bug

(In reply to comment #23)
> Created an attachment (id=27910) [review]
> Patch for the bug
> 
> This is the actual patch with all the changes.

You must set the "review?" flag on the patch to get it reviewed.

[David Kilzer (:ddkilzer)]

Comment on attachment 27910 [details]
Patch for the bug

This patch:

- Adds needless whitespace.
- Has no ChangeLog entries.
- Has no layout test.
- Uses tabs instead of spaces for indenting.
- Includes commented-out code.
- Is needless complex for this fix.

I'm sorry I can't go into more detail here, but see <http://webkit.org/coding/contributing.html> for some guidelines.

I really appreciate your effort to fix the bug, so please don't let this deter you from submitting patches in the future.  Thanks!

[David Kilzer (:ddkilzer)]

Created attachment 28881 [details]
Patch v2

Proposed fix.

[David Kilzer (:ddkilzer)]

Comment on attachment 28881 [details]
Patch v2

>+    const KURL baseURL = ((m_baseURL.isEmpty() || m_baseURL == blankURL()) && parentDocument()) ? parentDocument()->baseURL() : m_baseURL;

Oops!  This should be const KURL& baseURL.

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> Go to https://bugs.webkit.org/attachment.cgi?id=26694
> Type javascript:alert(frames[0].document.baseURI) in the address bar.
> Make sure it shows the same message as in Firefox.

After applying Patch v2 (Attachment #28881 [details]), Safari returns "about:blank" just like Firefox for this test case.  I'll add this to the layout test before landing.

Also, I should make the layout test fire an onload event in the iframe to finish the test rather than relying on a setTimeout() call.

[Darin Adler]

Comment on attachment 28881 [details]
Patch v2

> +    const KURL baseURL = ((m_baseURL.isEmpty() || m_baseURL == blankURL()) && parentDocument()) ? parentDocument()->baseURL() : m_baseURL;

This should be const KURL&, not const KURL.

r=me

[Darin Adler]

(In reply to comment #28)
> Also, I should make the layout test fire an onload event in the iframe to
> finish the test rather than relying on a setTimeout() call.

Yes. Both of those things sound good.

[Adam Barth]

Have you tested the case where we open a new window to "about:blanks"?  Does the opener URL matter, or does this magic only happen for subframes?

Have you tested the case where we open a new window to "about:blank" and then navigate the opener frame?  Are URLs completed using the original opener URL or the new opener URL?

[David Kilzer (:ddkilzer)]

(In reply to comment #31)
> Have you tested the case where we open a new window to "about:blanks"?  Does
> the opener URL matter, or does this magic only happen for subframes?
> 
> Have you tested the case where we open a new window to "about:blank" and then
> navigate the opener frame?  Are URLs completed using the original opener URL or
> the new opener URL?

I will test this, but my inclination is that a new window would not have a parentDocument() like an <iframe> or <frameset> element would have--because it's a separate window.

[Adam Barth]

> I will test this, but my inclination is that a new window would not have a
> parentDocument() like an <iframe> or <frameset> element would have--because
> it's a separate window.

I meant that we should test Firefox to see how it behaves in these cases and make sure we match.  :)

[David Kilzer (:ddkilzer)]

Created attachment 28908 [details]
Test cases for Comment #31

(In reply to comment #31)
> Have you tested the case where we open a new window to "about:blanks"?  Does
> the opener URL matter, or does this magic only happen for subframes?
> 
> Have you tested the case where we open a new window to "about:blank" and then
> navigate the opener frame?  Are URLs completed using the original opener URL or
> the new opener URL?

This attachment endeavors to test the above two cases.  I'm not sure if I completely understood what you were asking for, Adam, but I think this test case tests the above two issues.  The behavior is the same on Safari 4 Public Beta (with and without the patch to this bug) and Firefox 3.0.x.

[Adam Barth]

(In reply to comment #34)
> This attachment endeavors to test the above two cases.  I'm not sure if I
> completely understood what you were asking for, Adam, but I think this test
> case tests the above two issues.  The behavior is the same on Safari 4 Public
> Beta (with and without the patch to this bug) and Firefox 3.0.x.

Awesome.  Thanks for checking this.  It's not quite what I had in mind.  I was thinking something like:

function runTestLocationReplace()
{
    var theWindow = window.open("about:blank", "_blank");
    theWindow.document.location = "javascript:window.location.replace('/'); void(0)";
    return false;
}

I tested this, and it looks like we have the "right" behavior (i.e., we match Firefox 3).

It's interesting which things inherit from the opener as well as the parent and which things inherit only from the parent.  I haven't quite figured out what the rule is, but this patch appears to be correct.

[David Kilzer (:ddkilzer)]

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/frames/iframe-no-src-set-location-expected.txt
	A	LayoutTests/fast/frames/iframe-no-src-set-location.html
	A	LayoutTests/fast/frames/resources/iframe-no-src-set-location-pass.html
	M	WebCore/ChangeLog
	M	WebCore/dom/Document.cpp
Committed r41953

http://trac.webkit.org/changeset/41953