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

VERIFIED FIXED in Firefox 54

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
3 years ago
6 months ago

People

(Reporter: jryans, Assigned: bobowen)

Tracking

({regression})

Trunk
Firefox 55
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 fixed, firefox55 verified)

Details

(Whiteboard: sbwc2, sbmc2, sblc3)

Attachments

(5 attachments, 3 obsolete attachments)

Created attachment 8623263 [details]
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.
I was testing in Nightly 41.0a1 (2015-06-16).
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.
tracking-e10s: --- → ?
tracking-e10s: ? → m8+
Assignee: nobody → dtownsend
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.
Depends on: 1166949
No longer depends on: 1166949
Keywords: regressionwindow-wanted
Created attachment 8645945 [details] [diff] [review]
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.
Attachment #8645945 - Flags: feedback?(ttaubert)
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 => {
  [...]
});
Attachment #8645945 - Flags: feedback?(ttaubert) → feedback+
Created attachment 8647753 [details] [diff] [review]
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.
Attachment #8645945 - Attachment is obsolete: true
Attachment #8647753 - Flags: review?(wmccloskey)
Attachment #8647753 - Flags: review?(wmccloskey) → review+

Comment 7

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/76a433881e4b
https://hg.mozilla.org/mozilla-central/rev/76a433881e4b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

2 years ago
Depends on: 1213650
Depends on: 1204215
Depends on: 1214210
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]
Status: RESOLVED → VERIFIED
https://hg.mozilla.org/integration/fx-team/rev/f622e6d5fd171a3ef6456c393f1617ec9d7eb3a7
Backout fix for bug 1175267 (76a433881e4b) for causing bug 1213650.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Re-landing this because I found a solution that lets us keep it in the tree in bug 1213650.
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9410aa231e87
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED

Updated

2 years ago
Depends on: 1228179

Comment 14

2 years ago
This caused Bug 1228179 again.
The offending Bug 1175267 should be simply backed out!
https://hg.mozilla.org/integration/fx-team/rev/4eb9f0146a67a08c4999ab31bbaee83e90d96846
Backout patch for bug 1175267 (again) for causing bugs 1213650 and 1228179.
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
tracking-e10s: m8+ → ?
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]
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?
Flags: needinfo?(mconley)
Let's re-open this one.
Status: RESOLVED → REOPENED
Flags: needinfo?(mconley)
Resolution: FIXED → ---
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.
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
OS: Mac OS X → All
Hardware: Unspecified → All
Summary: about:addons page turns blank when opening XPI file → [e10s] about:addons page turns blank when opening XPI file
Version: unspecified → Trunk
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?
Assignee: dtownsend → nobody
Flags: needinfo?(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.
Flags: needinfo?(mconley)
I went over the STR with mconley and this clearly isn't a blocker for 49.
status-firefox49: affected → fix-optional

Updated

a year ago
tracking-e10s: ? → +
Component: Add-ons Manager → Tabbed Browser
Product: Toolkit → Firefox
Target Milestone: mozilla43 → ---
Hi Jim, given that we will be whitelisting a lot more add-ons in Beta50, is this something we can fix in Beta50 cycle?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
Tagging this as fix-optional based on Jim's reply in the previous comment.
status-firefox50: affected → fix-optional
status-firefox51: affected → fix-optional
status-firefox52: --- → fix-optional
status-firefox53: --- → fix-optional

Updated

8 months ago
Duplicate of this bug: 1342205
(Assignee)

Comment 28

8 months ago
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.
Assignee: nobody → bobowencode
Status: REOPENED → ASSIGNED
Whiteboard: sbwc2, sbmc2, sblc3
(Assignee)

Comment 29

8 months ago
Created attachment 8849493 [details] [diff] [review]
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.
Attachment #8849493 - Flags: review?(dtownsend)
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?
Attachment #8849493 - Flags: review?(dtownsend) → review+
(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.
Flags: needinfo?(bobowencode)
(Assignee)

Comment 32

8 months ago
(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.
Flags: needinfo?(bobowencode)
(Assignee)

Comment 33

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

I need to rebase this on top of bug 1351358.
(Assignee)

Comment 34

7 months ago
Created attachment 8866409 [details] [diff] [review]
Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser
Attachment #8866409 - Flags: review?(dtownsend)
(Assignee)

Updated

7 months ago
Attachment #8849493 - Attachment is obsolete: true
(Assignee)

Comment 35

7 months ago
Created attachment 8866410 [details] [diff] [review]
Part 2: Ensure that process does not switch for file:// XPI installs
Attachment #8866410 - Flags: review?(dtownsend)
(Assignee)

Updated

7 months ago
Attachment #8647753 - Attachment is obsolete: true
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 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.
Attachment #8866410 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 38

6 months ago
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 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.
Attachment #8866409 - Flags: review?(dtownsend) → review+

Comment 40

6 months ago
Pushed by bobowencode@gmail.com:
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
(Assignee)

Updated

6 months ago
status-firefox43: fixed → affected
status-firefox45: fixed → affected
status-firefox54: --- → affected
status-firefox55: --- → affected

Comment 41

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5116d9797e1
https://hg.mozilla.org/mozilla-central/rev/2eeca6aa6f45
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
status-firefox43: affected → ---
status-firefox45: affected → ---
status-firefox49: fix-optional → wontfix
status-firefox50: fix-optional → wontfix
status-firefox51: fix-optional → wontfix
status-firefox52: fix-optional → wontfix
status-firefox53: fix-optional → wontfix
status-firefox-esr52: --- → wontfix
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Please request uplift on this when you get a chance.  Thanks!
Flags: needinfo?(bobowencode)
(Assignee)

Comment 44

6 months ago
Created attachment 8871190 [details] [diff] [review]
* BETA * - Part 1: Handle file:// URI xpi files in chrome code directly instead of routing through content browser
(Assignee)

Comment 45

6 months ago
Created attachment 8871191 [details] [diff] [review]
* BETA * - Part 2: Ensure that process does not switch for file:// XPI installs
(Assignee)

Comment 46

6 months ago
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
Flags: needinfo?(bobowencode)
Attachment #8871190 - Flags: approval-mozilla-beta?
(Assignee)

Comment 47

6 months ago
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.
Attachment #8871191 - Flags: approval-mozilla-beta?
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.
Attachment #8871190 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 months ago
Attachment #8871191 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/7e720ada7954
https://hg.mozilla.org/releases/mozilla-beta/rev/41389795f394
status-firefox54: affected → fixed
Flags: in-testsuite+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.