Bug 145580

Summary: REGRESSION (r183498): Certain types of frame loads in iframes with <base target="_blank"> can open urls in new window/tabs
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, buildbot, commit-queue, japhet, mark.lam, rniwa, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1
none
Patch v2
mark.lam: review+, buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-mavericks
none
Patch v3 none

Description Brady Eidson 2015-06-02 17:04:14 PDT
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>
Comment 1 Brady Eidson 2015-06-02 17:16:52 PDT
Created attachment 254125 [details]
Patch v1
Comment 2 Mark Lam 2015-06-02 17:45:39 PDT
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?
Comment 3 Brady Eidson 2015-06-02 21:06:34 PDT
(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.
Comment 4 Brady Eidson 2015-06-02 21:24:56 PDT
The fragment scroll version in NavigationScheduler is, indeed, testable.

Updated patch coming shortly.
Comment 5 Brady Eidson 2015-06-02 22:52:37 PDT
Created attachment 254152 [details]
Patch v2
Comment 6 Build Bot 2015-06-02 23:27:32 PDT
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
Comment 7 Build Bot 2015-06-02 23:27:36 PDT
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
Comment 8 Brady Eidson 2015-06-03 09:22:27 PDT
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.
Comment 9 Brady Eidson 2015-06-03 09:26:57 PDT
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.
Comment 10 Brady Eidson 2015-06-03 09:29:41 PDT
It's just unimplemented stuff in WKTR. Will add it.
Comment 11 Mark Lam 2015-06-03 09:32:52 PDT
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?
Comment 12 Brady Eidson 2015-06-03 09:41:28 PDT
Created attachment 254182 [details]
Patch v3
Comment 13 WebKit Commit Bot 2015-06-03 09:44:10 PDT
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.
Comment 14 Brady Eidson 2015-06-03 10:08:20 PDT
(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.
Comment 15 Brady Eidson 2015-06-03 10:25:21 PDT
(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!
Comment 16 Brady Eidson 2015-06-03 10:34:06 PDT
http://trac.webkit.org/changeset/185155
Comment 17 Joseph Pecoraro 2015-06-03 11:35:58 PDT
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 18 Darin Adler 2015-06-03 14:27:39 PDT
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.