Bug 145580 – REGRESSION (r183498): Certain types of frame loads in iframes with <base target="_blank"> can open urls in new window/tabs

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

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 254125 [details]
Patch v1

[Mark Lam]

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?

[Brady Eidson]

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

[Brady Eidson]

The fragment scroll version in NavigationScheduler is, indeed, testable.

Updated patch coming shortly.

[Brady Eidson]

Created attachment 254152 [details]
Patch v2

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

It's just unimplemented stuff in WKTR. Will add it.

[Mark Lam]

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?

[Brady Eidson]

Created attachment 254182 [details]
Patch v3

[WebKit Commit Bot]

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.

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

http://trac.webkit.org/changeset/185155

[Joseph Pecoraro]

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.

[Darin Adler]

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.