1219495 - about:addons renders with huge "Server not found" page if you're offline, instead of placeholder content

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Be offline (e.g. disable your WiFi, unplug ethernet cable)
 2. Start Firefox (ideally with a fresh profile, but not necessary)
 3. Tools | Add-ons

ACTUAL RESULTS:
Big "Server not found" page:
===
Server not found

Firefox can't find the server at services.addons.mozilla.org.

    Check the address for typing errors such as ww.example.com instead of www.example.com
    If you are unable to load any pages, check your computer's network connection.
    If your computer or network is protected by a firewall or proxy, make sure that Nightly is permitted to access the Web.
===

This gives the implication that I can't configure my add-ons when I'm offline. (Really I can, if I ignore the error page & just use the sidebar on the left. But this is somewhat non-obvious)


EXPECTED RESULTS:
Some sort of locally-stored (or placeholder) content. Not something broken-looking like a server error page.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug (w/ brief flashes of placeholder content)

Here's a screencast of this bug's STR, followed by several reloads.

As you may be able to see in this screencast, it looks like there *is* some placeholder content (with text about "When you're connected to the internet..."), but it only flashes very briefly during loading, and is replaced by the "Server Not Found" page almost immediately.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: snapshot of placeholder content (this is shown *very* briefly during loading)

Comment 3

[Daniel Holbert [:dholbert]]

Browser version: Nightly 44.0a1 (2015-10-23)

Comment 4

[Tony Mechelynck [:tonymec]]

Attachment: "Get Add-ons" pane while browser is in offline mode in SeaMonkey

User agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41a1
Build identifier: 20151028003002

http://hg.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d
http://hg.mozilla.org/comm-central/rev/487b69abf1ec

Here is what I get for the "Get Add-ons" pane in SeaMonkey when "Work Offline" is selected. I think this is normal, IIUC it means that this pane requires web content from somewhere at addons.mozilla.org and cannot get it while offline.

All the other tabs of my add-ons manager display normally, even when offline.

Comment 5

[Daniel Holbert [:dholbert]]

Thanks for the response. I respectfully disagree that this is INVALID; reopening. Responses below.

(In reply to Tony Mechelynck [:tonymec] from comment #4)
> Here is what I get for the "Get Add-ons" pane in SeaMonkey when "Work
> Offline" is selected.

Right, you get an "offline mode, please go online" page. This is a little a more human-friendly than the "Server Not Found" page that I'm hitting.  But really, either error page seems pretty unexpected, as the landing-UI for a core piece of Firefox (/SeaMonkey / Thunderbird)'s offline profile-management functionality -- add-on management.

> I think this is normal

I disagree. Did you notice the splash page of placeholder content that I screenshotted? (which appears briefly before the error page) (attachment 8680344 [details])

It seems pretty likely to me that we *intend* to show that page when the user is offline. (but we're failing for some reason)  That would make much more sense than showing this error page.

> IIUC it means that this pane
> requires web content from somewhere at addons.mozilla.org and cannot get it
> while offline.

Clearly. But I don't think we intend to actually show this pane (the error pane) when offline.

> All the other tabs of my add-ons manager display normally, even when offline.

Same here. Still, Tools|Add-Ons takes me *here* by default, not to the other tabs. It's a pretty bad experience (for the average user) if this default landing page has a cryptic server-not-found UI.

Comment 6

[Tony Mechelynck [:tonymec]]

Attachment: "Get Add-ons" pane when online

Just for comparison, this is the same (obviously normal) pane when online.

FWIW, I don't see any flickering but this is probably due to the difference between "File → Work Offline" (where the browser knows that there is no connection) and cutting the connection unbeknownst to the browser (where it has to wait for a timeout).

Comment 7

[Tony Mechelynck [:tonymec]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Thanks for the response. I respectfully disagree that this is INVALID;
> reopening. Responses below.
> 
> (In reply to Tony Mechelynck [:tonymec] from comment #4)
> > Here is what I get for the "Get Add-ons" pane in SeaMonkey when "Work
> > Offline" is selected.
> 
> Right, you get an "offline mode, please go online" page. This is a little a
> more human-friendly than the "Server Not Found" page that I'm hitting.  But
> really, either error page seems pretty unexpected, as the landing-UI for a
> core piece of Firefox (/SeaMonkey / Thunderbird)'s offline
> profile-management functionality -- add-on management.
> 
> > I think this is normal
> 
> I disagree. [...]

I still think that this is either intended behaviour (i.e. INVALID), or else a bug, but not worth fixing (i.e. WONTFIX); but who am I to judge? I am just a QA volunteer, not an owner or peer of the relevant module. I see that Mossop (Toolkit owner) is already on the CC list; let's add Unfocused (Add-ons manager owner) and await their verdict.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8680344 [details]
snapshot of placeholder content (this is shown *very* briefly during loading)

The placeholder than Dan sees briefly is what we're meant to show whenever there is an error displaying the real site so something is broken here.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks, Mossop.

One other note: I can reproduce this at least as far back as Firefox 15, so this is not a recent regression (if it's a regression at all).

Comment 10

[Dave Townsend [:mossop]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Thanks, Mossop.
> 
> One other note: I can reproduce this at least as far back as Firefox 15, so
> this is not a recent regression (if it's a regression at all).

It's possible that we only checked the actual error cases and never thought about the offline case.

Comment 11

[Andy McKay]

I would like to fix this when we redo about:addons if possible.

Comment 12

[Faith Magcalas]

Hi! I need to work on a gfb for one of my classes, and I've chosen this one. I'll be creating a patch file as part of the assignment. Could someone guide me as to what files I should look at? Thanks.

Comment 13

[Faith Magcalas]

Is the problem just with case handling in extensions.js? Is there a way to stop the default error page from coming through there?

Comment 14

[Dave Townsend [:mossop]]

(In reply to Faith Magcalas from comment #13)
> Is the problem just with case handling in extensions.js? Is there a way to
> stop the default error page from coming through there?

The code handling this section is here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2040

It's meant to already be catching load errors and doing something appropriate: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2271

A good first step would be to investigate why that isn't working. It might be that when offline we get a status we aren't expecting.

Comment 15

[Faith Magcalas]

Attachment: First attempt.

I changed an if statement and it seems to work now. I ran the tests in browser_discovery.js and they pass. How else should I test it?

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8724115 [details] [diff] [review]
First attempt.

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2245,5 @@
>                          Ci.nsIWebProgressListener.STATE_IS_REQUEST |
>                          Ci.nsIWebProgressListener.STATE_TRANSFERRING;
>      // Once transferring begins show the content
> +    if ((aStateFlags & transferStart) && 
> +        (this.node.selectedPanel != this._error))

This solves one problem, but I think may cause others.

I think the better solution would be to make sure that all of the `transferStart` flags are present in `aStateFlags`, rather than checking that any of them are present.

The simplest way to do that is `((aStateFlags & transferStart) == transferStart)`. The fancy way to do it is `!(~aStateFlags & transferStart)`. Either should work.


As for how to test this, the relevant tests are in this file:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js

To test this, you first have to add a test which fails with the current code. The first step should be to set PREF_DISCOVERURL to a URL which should cause an error (see the test file for examples of setting the pref). A good choice would probably be "https://nocert.example.com/".

Then add a new test, based on one of the existing tests that call `isError()` to check for that error page after trying to load the discovery pane with the invalid URL. That part may be a bit tricky, so please let me know if you have any further questions.

Comment 17

[Andy McKay]

Had a chance to look at the feedback Faith?

Comment 18

[[e-mail]]

steal it

Comment 19

[[e-mail]]

Attachment: MozReview Request: Bug 1219495 - render placeholder content on add-ons page when offline; r?kmag

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

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8736175 [details]
MozReview Request: Bug 1219495 - render placeholder content on add-ons page when offline; r?kmag

https://reviewboard.mozilla.org/r/43157/#review40591

Looks good to me. Thanks!

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Please set the checkin-needed keyword if this is ready to land. Thanks!

Comment 22

[[e-mail]]

set!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1709e72eedc7

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1709e72eedc7