Bug 47978 – showModalDialog resolves relative URLs against the wrong base URL

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

[Andreas Kling]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

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.

[Daniel Nyström]

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?

[Adam Barth]

> 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.

[Daniel Nyström]

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.

[Adam Barth]

That surprises me, but that's certainly something we'd like to fix.

[Andreas Kling]

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

[Adam Barth]

Yeah, I thought I had analyzed this issue and picked the one that matched other browser already.  Do those tests pass in other browsers?

[Daniel Nyström]

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!

[Adam Barth]

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.

[Daniel Nyström]

(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!

[Daniel Nyström]

Created attachment 72305 [details]
A layout test testing URL resolving in modal dialogs

[Daniel Nyström]

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.

[Daniel Nyström]

Anything new on this?

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

Created attachment 73879 [details]
possible fix

[Adam Barth]

That patch might break these tests:

fast/frames/iframe-reparenting-new-page.html
fast/parser/no-crash-innerHTML-isindex.html

[Daniel Nyström]

Anything new on this? Could you Adam add "?" to review, maybe it'll get some attention?

[Daniel Nyström]

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.

[Alexey Proskuryakov]

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.

[Daniel Nyström]

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.

[Ahmad Saleem]

showModalDialog support is gone:

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

Do we need to fix this? or this can be "RESOLVED WONTFIX"?

[Ryosuke Niwa]

Yup, won't fix.