Closed Bug 1392210 Opened 7 years ago Closed 7 years ago

[OOP] onBeforeLinkTraversal is not called for extension frames

Categories

(WebExtensions :: Frontend, defect, P2)

defect

Tracking

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- verified
firefox57 --- verified

People

(Reporter: ke5trel, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(6 files)

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)."
Blocks: webext-oop
Keywords: regression
See Also: → 405592, 218205
This shouldn't be happening, will have to investigate.
Flags: needinfo?(mixedpuppy)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
This happens in browserAction as well, onBeforeLinkTraversal is not being called anywhere when remote=true.
Flags: needinfo?(mixedpuppy)
Summary: Links in sidebar open in sidebar instead of main window with OOP enabled → [OOP] onBeforeLinkTraversal is not called for extension frames
Kris, I'm not certain about all the additions here, but the link traversal is important and fixes this bug.
Assignee: nobody → mixedpuppy
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.
Attachment #8903285 - Flags: review?(kmaglione+bmo) → review+
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.
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
(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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0be1d1ac3bdc
extension content needs nsIWebBrowserChrome3 for link traversal, r=kmag
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
Attachment #8903285 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/0be1d1ac3bdc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Hi Florin, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Attached image 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?
Flags: needinfo?(florin.mezei) → needinfo?(mixedpuppy)
Attached file 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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(florin.mezei)
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
(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.
Attached image 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?
Flags: needinfo?(florin.mezei) → needinfo?(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.
Flags: needinfo?(mixedpuppy)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/925734099cb9
followup to handle clicks in subframes correctly, r=kmag
Good catch, thanks!
Flags: needinfo?(florin.mezei)
Depends on: 1397210
Blocks: 1397210
No longer depends on: 1397210
Comment on attachment 8903285 [details]
Bug 1392210 followup to handle clicks in subframes correctly,

Fix a regression. Beta56+.
Attachment #8903285 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image 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.
Flags: needinfo?(florin.mezei) → needinfo?(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.
Flags: needinfo?(mixedpuppy)
Blocks: 1397820
No longer blocks: 1397210
See Also: → 1398749
Will the last followup patch from Comment 29 be uplifted to beta?
Flags: needinfo?(kmaglione+bmo)
Ryan, can we get the followup uplifted?  Since it wasn't attached here, not sure how to beta?
Flags: needinfo?(kmaglione+bmo) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Attached image 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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.