Bug 47978 - showModalDialog resolves relative URLs against the wrong base URL
Summary: showModalDialog resolves relative URLs against the wrong base URL
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-20 05:46 PDT by Andreas Kling
Modified: 2022-07-31 02:29 PDT (History)
7 users (show)

See Also:


Attachments
A layout test testing URL resolving in modal dialogs (4.49 KB, patch)
2010-10-29 02:43 PDT, Daniel Nyström
abarth: review-
Details | Formatted Diff | Diff
possible fix (897 bytes, patch)
2010-11-15 02:03 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-10-20 05:46:20 PDT
When navigating through this test page:

http://www.timeterminal.se/qtwkbug/

The last page requested is "last.html" from a document with the URL /qtwkbug/bar/second.html
You'd expect it to open /qtwkbug/bar/last.html but it actually opens /qtwkbug/last.html

This appears to be a problem with relative URL resolution, Firefox and Chromium will use the calling document's base URL, Safari and QtWebKit use the outermost frame's base URL.

This behavior is caused by the following snippet in WebCore/bindings/generic/GenericBinding.h:

    // For historical reasons, we need to complete the URL using the
    // dynamic frame.
    Frame* frame = state->getFirstFrame();

If we change the getFirstFrame to getActiveFrame, we get the expected behavior.

There is probably indeed a reason we're doing this though, so I was hoping someone could shed some light on it.
Comment 1 Alexey Proskuryakov 2010-10-20 11:54:29 PDT
This code and comment were introduced this August in bug 41392. I think that a useless comment like this is a reason for r-, but it hasn't been called out in review.

Unfortunately, this bug was also hiding behind a [V8] prefix. Most of the code in it is supposed to be cross platform. It's important to make sure these prefixes don't lie.

I think that there is a chance to discover the reason for using getFirstFrame by changing it to getActiveFrame and running regression tests.
Comment 2 Adam Barth 2010-10-20 12:26:58 PDT
Sounds a comment I wrote.  Which frame we use is based on compatibility with other browsers and with the spec.  If you're considering changing this case, please cross-test with other browsers.
Comment 3 Daniel Nyström 2010-10-21 00:28:53 PDT
Hi. I was the one noticing this "odd" behavior (great thanks to Mr Kling with all help sorting it out and posting bug report).

My current task is to make a pretty old web site, developed around IE6, run on WebKit. Therefore it seems like Internet Explorer is also to be put next to Chrome and Firefox with the "last frame" behavior.

Also, changing from getFirstFrame() into getActiveFrame() would make window.showModalDialog() interchangable with window.open() i believe?
Comment 4 Adam Barth 2010-10-21 00:33:20 PDT
> My current task is to make a pretty old web site, developed around IE6, run on WebKit. Therefore it seems like Internet Explorer is also to be put next to Chrome and Firefox with the "last frame" behavior.

Does that mean these browsers all work the same in this regard?  If so, we're unlikely to change our behavior to be different from other browsers.
Comment 5 Daniel Nyström 2010-10-21 00:43:00 PDT
Yes, they all work the same. Currently, it seems like WebKit is the only one using the firstFrame() model instead of activeFrame().

AFAICT, a change in WebKit would result in WebKit behaving just as all the other big browsers on the market.
Comment 6 Adam Barth 2010-10-21 00:50:40 PDT
That surprises me, but that's certainly something we'd like to fix.
Comment 7 Andreas Kling 2010-10-21 06:37:35 PDT
The following tests break with the change:

fast/dom/Window/window-property-clearing.html
fast/frames/iframe-reparenting-new-page.html
http/tests/security/frameNavigation/context-for-location-assign.html
http/tests/security/frameNavigation/context-for-location-href.html
http/tests/security/frameNavigation/context-for-location.html
http/tests/security/frameNavigation/context-for-window-open.html
Comment 8 Adam Barth 2010-10-21 11:39:44 PDT
Yeah, I thought I had analyzed this issue and picked the one that matched other browser already.  Do those tests pass in other browsers?
Comment 9 Daniel Nyström 2010-10-25 09:53:24 PDT
Sorry for WOT but this is the tests run on Firefox 3.6.11 (Ubuntu 10.04):

/fast/dom/Window/window-property-clearing.html:

> Page 0:
> PASS: 'x' in childWindow should be true and is.
> PASS: childWindow.x should be 1 and is.
> PASS: 'f' in childWindow should be true and is.
> PASS: childWindow.f should be function and is.
> 
> Page 1:
> PASS: 'x' in childWindow should be false and is.
> PASS: childWindow.x should be undefined and is.
> PASS: 'f' in childWindow should be false and is.
> PASS: typeof childWindow.f should be undefined and is.
> PASS: 'name' in childWindow should be true and is.
> PASS: childWindow.name should be test and is.

But when reloaded with F5:

> Page 0:
> FAIL: 'x' in childWindow should be true but instead is false.
> FAIL: childWindow.x should be 1 but instead is undefined.
> FAIL: 'f' in childWindow should be true but instead is false.
> FAIL: childWindow.f should be function but instead is undefined.

> Page 1:
> PASS: 'x' in childWindow should be false and is.
> PASS: childWindow.x should be undefined and is.
> PASS: 'f' in childWindow should be false and is.
> PASS: typeof childWindow.f should be undefined and is.
> PASS: 'name' in childWindow should be true and is.
> PASS: childWindow.name should be test and is.

Reloading using Ctrl+F5 (ignore cache) makes them all pass every time.



/fast/frames/iframe-reparenting-new-page.html:

> PASS successfullyParsed is true

> TEST COMPLETE
> PASS Loaded iframe in window 1.
> PASS iframe.contentWindow.counter is 1
> PASS Loaded page 2.
> PASS Page 2 adopted the iframe.
> PASS Iframe transferred.
> FAIL iframe.contentWindow.counter should be 2. Was NaN.
> PASS window2.location.href is iframe.contentWindow.parent.location.href
> PASS Page 1 is closed.
> PASS Received the timer beat from the adopted iframe - exiting.



The frameNavigation tests is a bit unclear to me how to really test, but this is my remarks:

http/tests/security/frameNavigation/context-for-window-open.html:
 - No error messages in console (running Firebug).
 - What's shown: A blank iframe in a blank iframe on the main body.
 
 http/tests/security/frameNavigation/context-for-location.html:
 - Error output in console: frames[0].frames[0] is undefined (line 17)
 - What's shown: "document.referrer = http://../middle-frame-for-location.html" on body, and "Correct target." in the iframe within the iframe on the body.
 
 http/tests/security/frameNavigation/context-for-location-href.html:
 - Error output in console: frames[0].frames[0] is undefined (line 17)
 - What's shown: A blank iframe in a blank iframe on the main body.
 
 http/tests/security/frameNavigation/context-for-location-assign.html:
 - Error output in console: frames[0].frames[0] is undefined (line 17)
 - What's shown: "document.referrer = http://../middle-frame-for-location.html" on body, and "Correct target." in the iframe within the iframe on the body.



Is there anything else I can do to help? I'm new to most of things, even C++, so fixing all this is myself too much for me. But please let me know if there's anything I can do!
Comment 10 Adam Barth 2010-10-25 21:18:05 PDT
It sounds like these tests are correct because they pass in all browsers.  We need to understand how to achieve the behavior we want without breaking these tests.
Comment 11 Daniel Nyström 2010-10-26 03:14:40 PDT
(I though I just commited a comment but now it's gone? Sorry of any duplicates.)

I've updated http://www.timeterminal.se/qtwkbug/ to include an window.open() alternative. When using window.open() it all works. Even so on Chromium and Firefox.

It also opens all the pages using iframes, which also works in all browsers including webkit.

What else can I do to support a fix to this issue? Please let me know!
Comment 12 Daniel Nyström 2010-10-29 02:43:22 PDT
Created attachment 72305 [details]
A layout test testing URL resolving in modal dialogs
Comment 13 Daniel Nyström 2010-10-29 04:12:51 PDT
Comment on attachment 72305 [details]
A layout test testing URL resolving in modal dialogs

Note that this test will, in a way, fail on Chromium since it trigger a bug in which chromium will close the middle window first, then the last window. But the test is for URL resolving in modal windows, in which both Chromium and Firefox succeed.
Comment 14 Daniel Nyström 2010-11-07 23:54:37 PST
Anything new on this?
Comment 15 Adam Barth 2010-11-15 00:38:42 PST
Comment on attachment 72305 [details]
A layout test testing URL resolving in modal dialogs

Any particular reason this test is in the "harness" directory?  I'm not familiar with that directory.  Generally, this patch looks like its headed in the right direction, but all changes require a ChangeLog.  Please see http://webkit.org/coding/contributing.html.
Comment 16 Adam Barth 2010-11-15 01:43:59 PST
So, the situation here is that we have a nested message loop from showModalDialog and we're picking up the dynamic global objected based on the original message loop and not based on the nested message loop.  V8 avoids this bug because we "re-enter" V8, which resets the "entered" context to be the dynamic global object for the nested message loop.  We need to teach JSC to do the same.  DynamicGlobalObjectScope looks like a tempting mechanism.  Investigating.
Comment 17 Adam Barth 2010-11-15 02:03:53 PST
Created attachment 73879 [details]
possible fix
Comment 18 Adam Barth 2010-11-15 02:35:12 PST
That patch might break these tests:

fast/frames/iframe-reparenting-new-page.html
fast/parser/no-crash-innerHTML-isindex.html
Comment 19 Daniel Nyström 2010-11-23 02:39:16 PST
Anything new on this? Could you Adam add "?" to review, maybe it'll get some attention?
Comment 20 Daniel Nyström 2012-10-03 01:55:14 PDT
Any progress on this? It's still not working, and it's a major issue when you need to be compatible with old badly designed web sites.
Comment 21 Alexey Proskuryakov 2012-10-03 09:50:56 PDT
If you have multiple examples of real web sites broken due to this, please don't hesitate to post their URLs here. Discussion in this bug is all about synthetic test cases, which makes it extremely low priority.
Comment 22 Daniel Nyström 2012-10-03 23:47:01 PDT
I do understand your point of view. But in my case this is not primarily public web pages, but internal web services old and optimized for IE4-6, in my case time and attendance systems mostly. Since web browsing wasn't very common then they designed their web apps to behave like ordinary Windows apps with dialog windows being opened instead of page changes. That's where this bug apply.

Since there has been pretty much work on getting modal windows into Webkit, pretty recent too, I thought this bug would be of interest as well.
Comment 23 Ahmad Saleem 2022-07-31 02:25:50 PDT
showModalDialog support is gone:

https://github.com/WebKit/WebKit/commit/c4bb4d1af89e1ef5f57ef1b1cf7b65ce62c30ba5

Do we need to fix this? or this can be "RESOLVED WONTFIX"?
Comment 24 Ryosuke Niwa 2022-07-31 02:29:42 PDT
Yup, won't fix.