1175267 - [e10s] about:addons page turns blank when opening XPI file

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: blank-addons.png

STR:

1. Open about:addons in a tab
2. Choose File -> Open File...
3. Select some XPI you want to install

ER:

The add-on install doorhanger appears, with the about:addons page below it.

AR:

The doorhanger appears, but the page is now blank, with a title of "New Tab", but still has the URL "about:addons".

e10s is on.  I believe this is a relatively recent regression, but not sure.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I was testing in Nightly 41.0a1 (2015-06-16).

Comment 2

[Dave Townsend [:mossop]]

I'd expect this to have been around a while. We're probably flipping the browser remoteness to open the file: URI, then we realise its an extension to install so cancel the load and install it and don't flip the browser back to the about:addons page.

Comment 3

[Dave Townsend [:mossop]]

Flipping back to the add-ons manager is going to be hard. We do want to display a special in-content UI for file-system installs though so we can just do that here I think.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch

This is a general problem with the case where a new load switches a tabs process and then that load is cancelled without rendering any document, XPI installation is the main example and it can happen by just entering an XPI url into the address bar or with file - open.

Here is an approach that seems to work, wanted to see how sane it looks to someone cleverer than me. We check the status of the load started by the content restore code. If it is an abort then we throw away the new load info and simple restore the current history entry. This causes us to cascade to switching the tab back to the original process and reload the document there.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8645945 [details] [diff] [review]
patch

Review of attachment 8645945 [details] [diff] [review]:
-----------------------------------------------------------------

That seems reasonable, having a test would be great.

::: browser/components/sessionstore/ContentRestore.jsm
@@ +185,5 @@
>  
> +    let checkCallback = (status) => {
> +      // If a requested load was aborted then just restore the current history
> +      // entry
> +      if (loadArguments && (status == Cr.NS_BINDING_ABORTED)) {

Nit: Maybe elaborate some more that loadArgument!=null means navigation after remoteness switching - that was the only use case, wasn't it?

@@ +197,1 @@
>      // Listen for the tab to finish loading.

Nit: Maybe // Observe the the tab load (or something along those lines).

@@ +197,2 @@
>      // Listen for the tab to finish loading.
> +    this.restoreTabContentStarted(checkCallback);

Nit: I'd just do:

this.restoreTabContentStarted(status => {
  [...]
});

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch

Tim is out on PTO now, maybe you can review this Bill? Tim already ok'ed the approach and I've addressed his feedback above and added a test.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/76a433881e4b

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76a433881e4b

Comment 9

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 41.0a1(2015-06-16) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Aurora 44.0a2(2015-10-29)!

Build ID: 20151029045227
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20151030]

Comment 10

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/f622e6d5fd171a3ef6456c393f1617ec9d7eb3a7
Backout fix for bug 1175267 (76a433881e4b) for causing bug 1213650.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Re-landing this because I found a solution that lets us keep it in the tree in bug 1213650.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/9410aa231e873b2bd5878a8dea1e7e82f0ee3eca
Bug 1175267: When a load redirected to a new process is cancelled restore the existing content. r=billm

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9410aa231e87

Comment 14

[Alice0775 White]

This caused Bug 1228179 again.
The offending Bug 1175267 should be simply backed out!

Comment 15

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/4eb9f0146a67a08c4999ab31bbaee83e90d96846
Backout patch for bug 1175267 (again) for causing bugs 1213650 and 1228179.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Unfortunately, it seems my fix in bug 1213650 didn't fix the entirety of the problem - Alice0775 is still experiencing this quite often. My patch fixed _a_ problem, but not exactly the one Alice0775 was seeing.

Considering the symptoms of the bug[1] involve dataloss, I'm inclined to back this patch out (again) and investigate alternative solutions to this problem that doesn't end up re-opening bug 1213650 (or its clone bug 1228179).

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1213650#c0

Backed out: https://hg.mozilla.org/integration/fx-team/rev/4eb9f0146a67

Comment 17

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 41.0a1 (2015-06-16) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 43.0b7!

Build ID: 20151126120800
User Agent: Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0

[testday-20151127]

Comment 18

[Vasilica Tamas, QA [:vtamas]]

This issue is still reproducible on Firefox 51.0a1 (2016-08-29), Firefox 50.0a2 (2016-08-29) and Firefox 49.0b7 with e10s enabled under Windows 10 64-bit. 

Should I reopen this bug or file a new one?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Let's re-open this one.

Comment 20

[Vasilica Tamas, QA [:vtamas]]

This issue reproduces only with e10s enabled and I encountered it also for about:newtab, about:accounts, about:config, about:preferences and about:support so this is not an about:addons specific bug. Setting the tracking flags accordingly.

Comment 21

[Andy McKay]

I hope Mossop doesn't consider me too presumptuous, but I don't think he's working on it. mconley, any suggestions where this should live, assuming its not a Toolkit > Add-ons Manager bug?

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Hm.

If I understand what's going wrong properly, there's a bit of a freak-out going on because content starts to load a file:// URI, we flip remoteness from the non-remote page to remote (since file:// can be handled remotely), and then when we realize that we have a handler for the XPI file, we fail to switch back.

So I think this technically falls under the browser remoteness flipping machinery - some of which is in SessionStore.jsm, but the flipping itself is within browser.js and tabbrowser.xml.

So it's kinda spread around. Your choices are:

Firefox :: Tabbed Browser
Firefox :: Session Restore
or, if all else fails,
Firefox :: General

In any case, this will get triaged by the e10s team tomorrow due to the flag being set.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

I went over the STR with mconley and this clearly isn't a blocker for 49.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jim, given that we will be whitelisting a lot more add-ons in Beta50, is this something we can fix in Beta50 cycle?

Comment 25

[Jim Mathies [:jimm]]

(In reply to Ritu Kothari (:ritu) from comment #24)
> Hi Jim, given that we will be whitelisting a lot more add-ons in Beta50, is
> this something we can fix in Beta50 cycle?

no we won't be targeting 50 for a fix. This is pretty edge case, I don't think we need to track this.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tagging this as fix-optional based on Jim's reply in the previous comment.

Comment 28

[Bob Owen (:bobowen)]

I've got a couple of possible fixes for this.

One does a refresh of the page after the install, which looks pretty poor.
The other special cases XPIs in some of the drag and drop code, instead of sending down to the child, only to have it sent back up again.

I'll upload a patch for the second case to see what people think.

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 1: Handle dropped xpi files in chrome code directly instead of routing through content browser

mossop - not sure who is the best person to review drag and drop stuff.

Also, I'd like to write a test for this and although I can see a few tests that synthesize drag and drop of some sorts, I'm not sure they would trigger this code.
Any ideas on any test utils I can use for this.

Comment 30

[Dave Townsend [:mossop]]

Comment on attachment 8849493 [details] [diff] [review]
Part 1: Handle dropped xpi files in chrome code directly instead of routing through content browser

Review of attachment 8849493 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good but I'd really like to see a test for it if we can, can you try synthesizing the events, I'd expect it to work.

Any thoughts on handling the other cases?

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Bob Owen (:bobowen) from comment #28)
> I've got a couple of possible fixes for this.
> 
> One does a refresh of the page after the install, which looks pretty poor.
> The other special cases XPIs in some of the drag and drop code, instead of
> sending down to the child, only to have it sent back up again.
> 
> I'll upload a patch for the second case to see what people think.

While drag and drop is probably one avenue for this issue, comment 0 mentions File -> Open which (I am guessing...) doesn't go through a drag and drop path.

Just want make sure that's not getting lost here, in case they are separate issues.

Comment 32

[Bob Owen (:bobowen)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #31)

> While drag and drop is probably one avenue for this issue, comment 0
> mentions File -> Open which (I am guessing...) doesn't go through a drag and
> drop path.

Ah, right, I was originally looking at bug 1342205, I hadn't read this bug closely enough.

I might need to think about moving where we are picking this up, to try and cover other cases.
Or maybe we could re-use some of this code in the other cases.

Comment 33

[Bob Owen (:bobowen)]

Looking good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc45d0ff496156ddd6185103713c1fc22de35e53

I need to rebase this on top of bug 1351358.

Comment 34

[Bob Owen (:bobowen)]

Attachment: Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser

Comment 35

[Bob Owen (:bobowen)]

Attachment: Part 2: Ensure that process does not switch for file:// XPI installs

Comment 36

[Dave Townsend [:mossop]]

Comment on attachment 8866409 [details] [diff] [review]
Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser

Review of attachment 8866409 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1010,5 @@
> +                                               install);
> +        }, mimeType);
> +
> +        // If this tab has just been added it may not have a title set yet.
> +        gBrowser.setTabTitle(gBrowser.getTabForBrowser(aBrowser));

Why is this necessary?

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 8866410 [details] [diff] [review]
Part 2: Ensure that process does not switch for file:// XPI installs

Review of attachment 8866410 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/browser/browser_file_xpi_no_process_switch.js
@@ +1,4 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +
> +const addonInstallId = "addon-install-confirmation";

Capitalize the constant please, ADDON_INSTALL_ID.

@@ +43,5 @@
> +    is(Services.appinfo.processID, arg.expectedPid, arg.message);
> +  });
> +}
> +
> +function* testOpenedAndDraggedXPI(aBrowser) {

Use an async function here.

@@ +90,5 @@
> +                          "Check that browser has not switched process.");
> +}
> +
> +// Test for bug 1175267.
> +add_task(function* () {

Use an async function here.

Comment 38

[Bob Owen (:bobowen)]

Thanks for the reviews.

(In reply to Dave Townsend [:mossop] from comment #36)
> Comment on attachment 8866409 [details] [diff] [review]
> Part 1: Handle file:// URI xpi files in chrome code directly instead of

> > +        // If this tab has just been added it may not have a title set yet.
> > +        gBrowser.setTabTitle(gBrowser.getTabForBrowser(aBrowser));
> 
> Why is this necessary?

Well, when I originally tested this I used to get a tab with a blank name.
When I added this line it gave me New Tab, which is the same as release.
But now I get file:///<whatever>.xpi either way, so I'll remove it. :-)

(In reply to Dave Townsend [:mossop] from comment #37)
> Comment on attachment 8866410 [details] [diff] [review]
> Part 2: Ensure that process does not switch for file:// XPI installs

Thanks, changes made locally, also changed the yields to awaits.

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 8866409 [details] [diff] [review]
Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser

r+ with that removed.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5116d9797e1
Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser. r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eeca6aa6f45
Part 2: Ensure that process does not switch for file:// XPI installs. r=mossop

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a5116d9797e1
https://hg.mozilla.org/mozilla-central/rev/2eeca6aa6f45

Comment 42

[Simona Badau, Desktop QA]

Reproduced the initial issue using Nightly 55.0a1 (Build ID: 20170510030301) on Windows 10 x64.

Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170523030206) on Windows 10 x64, Ubuntu 16.04  x64 and Mac OS X 10.12.

Comment 43

[Nathan Froyd [:froydnj]]

Please request uplift on this when you get a chance.  Thanks!

Comment 44

[Bob Owen (:bobowen)]

Attachment: * BETA * - Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser

Comment 45

[Bob Owen (:bobowen)]

Attachment: * BETA * - Part 2: Ensure that process does not switch for file:// XPI installs

Comment 46

[Bob Owen (:bobowen)]

Comment on attachment 8871190 [details] [diff] [review]
* BETA * - Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser

Needed a small rebase because of bug 1351358.

Approval Request Comment
[Feature/Bug causing the regression]:
e10s

[User impact if declined]:
Users installing XPIs using file:// URIs dragged into or otherwise opened in an existing non-remote page (like about:addons) will get a blank page, because of an unnecessary process-switch.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
I don't think so, I had to rebase slightly, so I've compiled and tested this manually and with the tests from part 2 (with and without e10s).

[List of other uplifts needed for the feature/fix]:
Beta Part 2 from this bug.

[Is the change risky?]:
I'd say small to medium risk.

[Why is the change risky/not risky?]:
Mainly because, although the change is relatively simple, it is in code that is used by nearly all loads triggered through the UI.

[String changes made/needed]:
None

Comment 47

[Bob Owen (:bobowen)]

Comment on attachment 8871191 [details] [diff] [review]
* BETA * - Part 2: Ensure that process does not switch for file:// XPI installs

Again a small rebase.

See comment 46.

Comment 48

[Gerry Chang [:gchang]]

Comment on attachment 8871190 [details] [diff] [review]
* BETA * - Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser

Fix a long standing regression and was verified. Beta54+. Should be in 54 beta 11.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e720ada7954
https://hg.mozilla.org/releases/mozilla-beta/rev/41389795f394

Comment 50

[Andrei Vaida [:avaida]]

(In reply to Bob Owen (:bobowen) from comment #46)
> [Is this code covered by automated tests?]:
> Yes
> 
> [Has the fix been verified in Nightly?]:
> Yes
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> I don't think so, I had to rebase slightly, so I've compiled and tested this
> manually and with the tests from part 2 (with and without e10s).

Setting qe-verify- based on Bob's assessment on manual testing needs and the fact that this fix has automated coverage.