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

VERIFIED FIXED in Firefox 54

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: jryans, Assigned: bobowen)

Tracking

({regression})

Trunk
Firefox 55
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)

Reporter

Description

4 years ago
Posted image 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.
Reporter

Comment 1

4 years ago
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: --- → ?
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
Posted patch patch (obsolete) — Splinter Review
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+
Posted patch patch (obsolete) — Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/76a433881e4b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

4 years ago
Depends on: 1213650
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
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

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9410aa231e87
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 1228179

Comment 14

4 years ago
This caused Bug 1228179 again.
The offending Bug 1175267 should be simply backed out!
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
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.
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

Comment 21

3 years ago
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.

Updated

3 years ago
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.
Duplicate of this bug: 1342205
Assignee

Comment 28

2 years 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

2 years ago
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+
Reporter

Comment 31

2 years ago
(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

2 years 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

Updated

2 years ago
Attachment #8849493 - Attachment is obsolete: true
Assignee

Updated

2 years 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

2 years 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

2 years 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

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5116d9797e1
https://hg.mozilla.org/mozilla-central/rev/2eeca6aa6f45
Status: ASSIGNED → RESOLVED
Closed: 4 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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
Please request uplift on this when you get a chance.  Thanks!
Flags: needinfo?(bobowencode)
Assignee

Comment 46

2 years 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

2 years 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+
Attachment #8871191 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.