1392210 - [OOP] onBeforeLinkTraversal is not called for extension frames

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[Kestrel]

Extension sidebar links are now loading in the sidebar rather than the main window since OOP was enabled, contradicting past behavior.

Bug 218205 Comment 9:

> "A Web panel's links should target the main content area.  Do this if no
> modifier keys are down and if there's no target or the target equals _main (the
> IE convention) or _content (the Mozilla convention)."

Comment 1

[Shane Caraveo (:mixedpuppy)]

This shouldn't be happening, will have to investigate.

Comment 2

[Shane Caraveo (:mixedpuppy)]

This happens in browserAction as well, onBeforeLinkTraversal is not being called anywhere when remote=true.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1392210 followup to handle clicks in subframes correctly,

Review commit: https://reviewboard.mozilla.org/r/175076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175076/

Comment 4

[Shane Caraveo (:mixedpuppy)]

Kris, I'm not certain about all the additions here, but the link traversal is important and fixes this bug.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

https://reviewboard.mozilla.org/r/175076/#review180734

r=me for the `onBeforeLinkTraversal` change, but not for the `redirectLoad` changes. If we want to handle that, we'll probably have to do so by forcing those links to open in new tabs.

::: toolkit/components/extensions/ext-browser-content.js:301
(Diff revision 1)
> +    if (!E10SUtils.shouldLoadURI(docShell, URI, referrer, hasPostData)) {
> +      E10SUtils.redirectLoad(docShell, URI, referrer, triggeringPrincipal, false);
> +      return false;
> +    }

This won't work, since our <browser>s don't support changing processes this way.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175076/diff/1-2/

Comment 7

[Shane Caraveo (:mixedpuppy)]

Well, that broke tests.  

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89521746c01f1bbec86f97c8a0d6835b45075236&selectedJob=128038459

Seems shouldLoadURI needs to return true here regardless, tests then pass with remote extensions.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175076/diff/2-3/

Comment 9

[Kris Maglione [:kmag]]

That test just fails because this change prevents loading a web URL at the top level in the popup. Honestly, that's probably the behavior that we want.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #9)
> That test just fails because this change prevents loading a web URL at the
> top level in the popup. Honestly, that's probably the behavior that we want.

That would introduce a difference of behavior between remote and non-remote.  If we want to do that, I'd rather do it in a followup and get it consistent between the two.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0be1d1ac3bdc
extension content needs nsIWebBrowserChrome3 for link traversal, r=kmag

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

Early request lest I forget...putting this out there for consideration.

Approval Request Comment
[Feature/Bug causing the regression]: remote webextensions
[User impact if declined]: on windows where we have set webextensions.remote=true, extension content with links to external sites will load external sites in the extension panel (browser/page/sidebar actions)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: load an extension with a sidebar, the sidebar has a simple link to a website (e.g. mozilla.org).  click on it, it should open in a new tab, not in the sidebar.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: This introduces the nsIWebBrowserChrome3 interface for remote extension panels.  For 56, this is limited to extension panels on windows.
[String changes made/needed]: none

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0be1d1ac3bdc

Comment 14

[Gerry Chang [:gchang]]

Hi Florin, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!

Comment 15

[Vasilica Tamas, QA [:vtamas]]

Attachment: sidebarlinks.gif

This issue is still reproducible on Firefox 57.0a1 (2017-09-04) under Windows 10 64-bit and Mac OS X 10.12.3. The extensions sidebar links are still loaded in the sidebar when OOP is enabled.
See attached screencast.

Shane, any thoughts about this?

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: isAppTab.xpi

Nightly works perfectly for me.  I'm not sure how you produced a sidebar with yahoo login to start with.  So I'll attach a dead simple addon.  This illustrates the problem.  With extensions.webextensions.remote=true. The mozilla link opens in a tab as expected on nightly.  Prior to 9-4 the side would open in the panel instead.

Link traversal has logic to figure out stuff about what it in the sidebar.  Part of that is whether it is same-origin-ish.  Since your video starts on yahoo, and you click on a yahoo link, I don't expect that link to open externally.

Comment 17

[Kestrel]

Your extension works as expected but "URL in a Box" still load links in the sidebar, eg set the sidebar to google.com, do a search and click on search results.

https://addons.mozilla.org/en-US/firefox/addon/url-in-a-box

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Kestrel from comment #17)
> Your extension works as expected but "URL in a Box" still load links in the
> sidebar, eg set the sidebar to google.com, do a search and click on search
> results.
> 
> https://addons.mozilla.org/en-US/firefox/addon/url-in-a-box

That's because google isn't doing a link traversal.  They are handling the links in js, probably doing a redirect after some kind of logging.  Pin a google tab and try the same thing, you'll see that the page loads into the pinned tab rather than a new tab.  Not much we can do about that, link traversal handling doesn't account for stuff like that, you'll have to do more work in those cases.

Comment 19

[Vasilica Tamas, QA [:vtamas]]

Attachment: oop-on-off.gif

I confirm that “mozilla” link is successfully opened in the main window while using the extension from https://bugzilla.mozilla.org/show_bug.cgi?id=1392210#c16. Tested on Firefox 57.0a1 (2017-09-04/05) under Windows 10 64-bit and Mac OS X 10.12.3.

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> I'm not sure how you produced a sidebar with yahoo login to start with.  

Installing the following webextension https://addons.mozilla.org/en-US/firefox/addon/yahoo-mail-sidebar/?src=search will automatically open the yahoo login page in the sidebar. 
I am also able to reproduce this issue using https://addons.mozilla.org/en-US/firefox/addon/sidebar-for-google-search/?src=search .
Those links are opened in main window while extensions.webextensions.remote is set to false and they are opened in the sidebar when oop is enabled. See attached video.


Is this a different bug that must be tracked separately?

Comment 20

[Shane Caraveo (:mixedpuppy)]

Ok, those sidebar addons are using an iframe to load the websites.  That is throwing off our link traversal handling (as it also does for pinned tabs).  I created bug 1396988 for pinned tabs, will create a followup patch here.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175076/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/925734099cb9
followup to handle clicks in subframes correctly, r=kmag

Comment 23

[Shane Caraveo (:mixedpuppy)]

Good catch, thanks!

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/925734099cb9

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

Fix a regression. Beta56+.

Comment 26

[Vasilica Tamas, QA [:vtamas]]

Attachment: sidebarissue.gif

Tested again on Firefox 57.0a1 (20170906220306) under Windows 10 64-bit and Mac OS X 10.12.3 and noticed that for https://addons.mozilla.org/en-US/firefox/addon/yahoo-mail-sidebar/?src=search the “Trouble signing in?” and “Sign up” links are still opened in the sidebar with oop enabled (this behavior is not encountered with oop disabled). 
The rest of the links are correctly opened in the main window. 

See attached screenshot.

Comment 27

[Shane Caraveo (:mixedpuppy)]

The correct behavior is that a same-origin url will remain in the panel (either pinned or extension panel).  disimilar origins will pop out to a tab.

So login.yahoo.com linking to login.yahoo.com will remain in the panel.

You will see that behavior if you pin a mail.yahoo.com tab.

There is a similar bug where link traversal is not happening correctly for non-remote extension panels when using iframes this way.  I can cause the same issue with the google sidebar when non-remote.

Making a new bug for that.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a6699a52aec

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3854a56016ce423ecccf4f03ca35b50218452d
Bug 1392210: Follow-up: Fix missing Services.jsm import. r=trivial

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9a3854a56016

Comment 31

[Vasilica Tamas, QA [:vtamas]]

Will the last followup patch from Comment 29 be uplifted to beta?

Comment 32

[Shane Caraveo (:mixedpuppy)]

Ryan, can we get the followup uplifted?  Since it wasn't attached here, not sure how to beta?

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4a923590e3e1

Comment 34

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

I confirm that this issue is fixed on Firefox 57.0a1 (20170917220255) and Firefox 56.0b12 (20170914024831) under Windows 10 64-bit and Mac OS X 10.12.3. Extension sidebar links are successfully opened in the main window and same-origin urls are displayed in the sidebar.
See attached screencast.