Bug 23310

Summary: Setting an absolute path (/abs) on an <iframe> with no src doesn't resolve the URL properly
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Page LoadingAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aegolden, ap, darin, ian, sam, vikramhegden
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Test case
none
Test case (no JavaScript)
none
Screenshots of the behaviour in firefox and safari
none
screen shots taken before CLICK ME
none
Patch for the bug
none
Patch for the bug
none
Patch for the bug
ddkilzer: review-
Patch v2
darin: review+
Test cases for Comment #31 none

Description David Kilzer (:ddkilzer) 2009-01-13 17:13:28 PST
* 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".
Comment 1 David Kilzer (:ddkilzer) 2009-01-13 17:14:02 PST
Created attachment 26694 [details]
Test case
Comment 2 David Kilzer (:ddkilzer) 2009-01-13 17:16:01 PST
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.
Comment 3 David Kilzer (:ddkilzer) 2009-01-13 17:17:50 PST
(In reply to comment #1)
> Created an attachment (id=26694) [review]
> Test case

This works as expected in FIrefox 3.
Comment 4 Adam Barth 2009-01-13 17:19:26 PST
Strange.  In Firefox 3 javascript:alert(frames[0].document.baseURI) alerts "about:blank" but when they resolve "/" they use a different baseURI.
Comment 5 David Kilzer (:ddkilzer) 2009-01-26 16:22:46 PST
Alexey, do any of your recent KURL changes affect the test case on this bug?
Comment 6 Alexey Proskuryakov 2009-01-27 00:05:54 PST
No - as Adam says, this is an issue with what base URL is used, not with parsing.
Comment 7 Alexey Proskuryakov 2009-01-27 10:49:38 PST
Do these changes or existing text cover this already? <http://html5.org/tools/web-apps-tracker?from=2710&to=2711>
Comment 8 Adam Barth 2009-01-27 10:57:48 PST
(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).
Comment 9 David Kilzer (:ddkilzer) 2009-01-27 11:50:07 PST
(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>
Comment 10 Ian 'Hixie' Hickson 2009-01-27 14:42:09 PST
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.
Comment 11 vikram hegde 2009-02-20 03:15:05 PST
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.
Comment 12 David Kilzer (:ddkilzer) 2009-02-20 07:37:13 PST
(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.
Comment 13 Adam Barth 2009-02-20 09:56:20 PST
(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.

Comment 14 vikram hegde 2009-02-23 21:17:38 PST
>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.




Comment 15 Adam Barth 2009-02-23 21:40:44 PST
(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.
Comment 16 vikram hegde 2009-02-23 22:30:28 PST
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.
Comment 17 Adam Barth 2009-02-23 23:20:50 PST
(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".
Comment 18 vikram hegde 2009-02-23 23:45:13 PST
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.
Comment 19 Adam Barth 2009-02-24 00:05:29 PST
(In reply to comment #18)
> screen shots taken before CLICK ME

Those look great, thanks!

Comment 20 vikram hegde 2009-02-24 01:14:08 PST
Created attachment 27907 [details]
Patch for the bug

The attachment is the patch for the mentioned bug.
Comment 21 Adam Barth 2009-02-24 01:20:39 PST
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.
Comment 22 vikram hegde 2009-02-24 02:07:03 PST
Created attachment 27909 [details]
Patch for the bug

Patch for the bug in diff format.
Comment 23 vikram hegde 2009-02-24 02:18:38 PST
Created attachment 27910 [details]
Patch for the bug

This is the actual patch with all the changes.
Comment 24 David Kilzer (:ddkilzer) 2009-02-24 03:36:32 PST
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.
Comment 25 David Kilzer (:ddkilzer) 2009-03-23 18:05:45 PDT
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!
Comment 26 David Kilzer (:ddkilzer) 2009-03-23 18:10:07 PDT
Created attachment 28881 [details]
Patch v2

Proposed fix.
Comment 27 David Kilzer (:ddkilzer) 2009-03-24 04:20:14 PDT
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.
Comment 28 David Kilzer (:ddkilzer) 2009-03-24 04:41:03 PDT
(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.
Comment 29 Darin Adler 2009-03-24 09:02:38 PDT
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
Comment 30 Darin Adler 2009-03-24 09:13:35 PDT
(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.
Comment 31 Adam Barth 2009-03-24 09:23:00 PDT
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?
Comment 32 David Kilzer (:ddkilzer) 2009-03-24 11:22:54 PDT
(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.
Comment 33 Adam Barth 2009-03-24 11:46:14 PDT
> 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.  :)
Comment 34 David Kilzer (:ddkilzer) 2009-03-24 13:22:52 PDT
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.
Comment 35 Adam Barth 2009-03-24 13:38:53 PDT
(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.
Comment 36 David Kilzer (:ddkilzer) 2009-03-24 14:41:47 PDT
$ 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