REGRESSION (r183498): Certain types of frame loads in iframes with <base target="_blank"> can open urls in new window/tabs In radar as <rdar://problem/20844292>
Created attachment 254125 [details] Patch v1
Comment on attachment 254125 [details] Patch v1 In the r183498 patch, the "_self" frame name was set by FrameLoader::changeLocation(), and FrameLoader::changeLocation() was called from 8 locations: 1 in InspectorFrontendClientLocal.cpp 1 in InspectorPageAgent.cpp 5 in NavigationScheduler.cpp 1 in DOMWindow.cpp This patch changes 4 cases in NavigationScheduler.cpp. I see that the ones in InspectorFrontendClientLocal.cpp, InspectorPageAgent.cpp, and the 5th case in NavigationScheduler.cpp are now explicitly given emptyString() as the frame name. The one in DOMWindow.cpp doesn't doesn't specify a frame name (uses the default empty String() of the FrameLoadRequest constructor). Are these intentional?
(In reply to comment #2) > Comment on attachment 254125 [details] > Patch v1 > > In the r183498 patch, the "_self" frame name was set by > FrameLoader::changeLocation(), and FrameLoader::changeLocation() was called > from 8 locations: > 1 in InspectorFrontendClientLocal.cpp > 1 in InspectorPageAgent.cpp > 5 in NavigationScheduler.cpp > 1 in DOMWindow.cpp > > This patch changes 4 cases in NavigationScheduler.cpp. > > I see that the ones in InspectorFrontendClientLocal.cpp, > InspectorPageAgent.cpp, and the 5th case in NavigationScheduler.cpp are now > explicitly given emptyString() as the frame name. The one in DOMWindow.cpp > doesn't doesn't specify a frame name (uses the default empty String() of the > FrameLoadRequest constructor). Are these intentional? The other 4 are also mistakes, and I overlooked them in this patch because of confusion thinking ""/emptyString() was the frame name when it fact it was the referer. The inspector ones are likely irrelevant because inspector markup won't ever have a <base> tag. They should still be restored to the old behavior, of course. The DOM window one cannot possibly be tested because it's in createWindow, and createWindow will never be for a frame with an existing document, and therefore there can never be a <base> tag. It should still be restored to the old behavior, of course. Because of a happy accident in the order of operations in FrameLoader::loadURL, the 5th NavigationScheduler one *should* be testable. Will take a look.
The fragment scroll version in NavigationScheduler is, indeed, testable. Updated patch coming shortly.
Created attachment 254152 [details] Patch v2
Comment on attachment 254152 [details] Patch v2 Attachment 254152 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4819418496368640 New failing tests: fast/loader/fragment-navigation-base-blank.html
Created attachment 254157 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Trivia: While working on fixing these, I noticed https://bugs.webkit.org/show_bug.cgi?id=145607 which is pretty much the same symptom caused by this regression... but's been around forever. Hence why I filed it separately.
This layout test failure is caused by differences in frame load delegate callbacks in WK1 vs WK2. I'll explore briefly. If the failure is because WK1 sends this callback but WK2 doesn't, I'll simply remove the frame load delegate calls from this patch as they are not strictly necessary. I'll then file a followup bug. If the failure is because DRT dumps something the WKTR doesn't, I'll include a change to WKTR.
It's just unimplemented stuff in WKTR. Will add it.
Comment on attachment 254152 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=254152&action=review r=me. The code change looks good to me. I’m not too familiar with the functionality exercised by the tests. Might want to get a second pair of eyes on that. I presume that they demonstrate the problem before your fix. Hence, they are good for the purpose of this patch. As for the mac-ews failures, the status says "Unable to pass tests without patch (tree is red?)”. It not be related to this patch. > Source/WebCore/ChangeLog:15 > + Later on, FrameLoader gives load requests without a frame name the <base> target if there is one, which could frame name *in* the <base> target?
Created attachment 254182 [details] Patch v3
Attachment 254182 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1012: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #13) > Attachment 254182 [details] did not pass style-queue: > > > ERROR: Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1012: > Extra space before last semicolon. If this should be an empty statement, use > { } instead. [whitespace/semicolon] [5] > Total errors found: 1 in 22 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. Will fix before landing, won't bother uploading a new patch for review.
(In reply to comment #11) > Comment on attachment 254152 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254152&action=review > > r=me. The code change looks good to me. I’m not too familiar with the > functionality exercised by the tests. Might want to get a second pair of > eyes on that. I presume that they demonstrate the problem before your fix. > Hence, they are good for the purpose of this patch. Yup, that should be the invariant for every test. Failing without the code change, passing with it. :) > > As for the mac-ews failures, the status says "Unable to pass tests without > patch (tree is red?)”. It not be related to this patch. That wasn't related, but there was a real failure in WK1, hence the addition to WKTR. Thanks for the review!
http://trac.webkit.org/changeset/185155
Comment on attachment 254182 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=254182&action=review > Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:226 > + FrameLoadRequest frameRequest(mainFrame.document()->securityOrigin(), resourceRequest, "_self", LockHistory::No, LockBackForwardList::No, MaybeSendReferrer, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Allow, ReplaceDocumentIfJavaScriptURL, ShouldOpenExternalURLsPolicy::ShouldNotAllow); Nit: These strings could be ASCIILiteral("self") since it is a string literal being converted to a WTF::String.
Comment on attachment 254182 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=254182&action=review >> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:226 >> + FrameLoadRequest frameRequest(mainFrame.document()->securityOrigin(), resourceRequest, "_self", LockHistory::No, LockBackForwardList::No, MaybeSendReferrer, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Allow, ReplaceDocumentIfJavaScriptURL, ShouldOpenExternalURLsPolicy::ShouldNotAllow); > > Nit: These strings could be ASCIILiteral("self") since it is a string literal being converted to a WTF::String. I also think they could/should be a some kind of string constant so we avoid the possibility of a typo.