487341 - accommodate unavailable network on Firefox start

Status: RESOLVED FIXED

(Mozilla Labs Graveyard :: Personas Plus, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

If the network is unavailable when Firefox starts (or possibly when a user opens a new Firefox window, depending on how the memory cache behaves), Personas doesn't successfully load and show the images for the selected persona (although it still applies the text and accent colors).  Nor does it load them when the network becomes available again, although it does load them for any new windows you create after the network has become available again.

Personas should accommodate the possibility that the network will be unavailable when Firefox starts, perhaps by caching the images locally, or watching for the "network back up" event and reloading the persona at that time, or via some other technique.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Note: this is also a problem for downloading of the data file, and it's also a problem when the network connection is apparently up but the user hasn't yet authenticated with the ISP via some web page (in which case, when Personas tries to load resources, those resources may get replaced by the ISP with the ISP's authentication page).

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The best short-term solution to this would be to cache the image files for the selected persona along with the data files containing general information about personas and the user's favorites.

The cache should live within a personas/cache/ directory inside the user's profile directory.  Personas should load the cached versions of the files on startup rather than loading them from the network.

It should continue to attempt to refresh the data files on a daily basis, however, updating the cache each time it successfully retrieves one of them.  And it should start attempting to refresh the image files on a daily basis, updating the cache each time it successfully retrieves those files.

The extension should ship with the latest version of the data file containing general information about personas along with the image files for the initial persona, so those files are available on first run.  I'm not sure how that'll work for BYOB/bundled builds, so cc:ing Kev for his input.

Cache invalidation will also be tricky, especially considering that we can't rely on the names of the image files being consistent, since they have arbitrary names.  We can canonicalize them to some extent when we store them in the cache, changing their names to header.* and footer.*, respectively, but their file extensions will still vary, since we support multiple image file formats.

Also note bug 516013, which is about caching the image files for the implementation of Personas that is being uplifted into the next version of Firefox.

cc:ing cbeard and Suneel, with whom I've had conversations about this fix over the last couple days, for their input.

Comment 3

[Kev Needham [:kev]]

per comment #2, I'd prefer we didn't pre-cache anything with the extension because of the potential impacts it has with non-default personas in a bundled install of any kind. While we could definitely pre-load extensions with different dynamic content, managing the extension content requires a bit of finangling (not huge), and you'd have variable content in the extension itself, which I'm not sure is something we want.

I'm a little unclear on when the content would be unavailable - would it just be the first time the browser is run, or does it check for the image data every time it starts up, and clears things out if the network/server is unavailable?

Does it make sense to consider a routine which, if the browser can't connect to the personas server to grab a non-default persona, displays an info bar to the user explaining it couldn't connect, and will try again whenever, and have the extension track if it successfully connected to the personas site and continue to try at a tbd interval? That way the user is aware there's a netcomm issue, and can take steps to correct it or understand why things aren't as expected or have changed.

That said, I think we can modify the way we do repacks to incorporate dynamic content, but I'd like to make sure that doesn't have any effects on version upgrades to the extension (i.e. what happens when an upgrade is installed to the profile directory vs. from an appdir install; will the cache files, regardless of whether they're installed in the appdir or profile, be copied to a cache location in the profile that's independent of the extension, etc.). 

If the cache directory will always be in the profile directory and independent from the extension itself, and can be pre-stuffed from data included with the extension on first-run, then I think we can accommodate it.

The only other thing I'd want to make sure of would be to check the effects of having different files in the extensions/personas directory from the AMO version on an update.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> per comment #2, I'd prefer we didn't pre-cache anything with the extension
> because of the potential impacts it has with non-default personas in a bundled
> install of any kind. While we could definitely pre-load extensions with
> different dynamic content, managing the extension content requires a bit of
> finangling (not huge), and you'd have variable content in the extension itself,
> which I'm not sure is something we want.

Is it easier to include files within the bundle but outside the extension?  Since bundles can create preferences, we could make the extension look for preferences that point to the locations of the files relative to the application installation directory.

They could even live in another extension that the bundle includes, like a "bundle" extension that registers a chrome location (f.e. chrome://bundle/content/) in which the extension looks for the files before using its own defaults.

> I'm a little unclear on when the content would be unavailable - would it just
> be the first time the browser is run, or does it check for the image data every
> time it starts up, and clears things out if the network/server is unavailable?

The issue with the default persona is mostly about first run, but caching of the image files is important for every startup, since that's when we apply the persona to the windows, and the network may not be available then.


> Does it make sense to consider a routine which, if the browser can't connect to
> the personas server to grab a non-default persona, displays an info bar to the
> user explaining it couldn't connect, and will try again whenever, and have the
> extension track if it successfully connected to the personas site and continue
> to try at a tbd interval? That way the user is aware there's a netcomm issue,
> and can take steps to correct it or understand why things aren't as expected or
> have changed.

The problem with that approach is that there isn't necessarily a network issue.  Some users have network configurations that don't connect to the network until after they start their browser, i.e. it is the initial network requests made by the browsers that connect the users to the network.

And even so, automagically displaying their personas seems better than notifying them about a network problem displaying their personas, since it "just works" and doesn't expose users to the machine details of the feature's implementation.


> That said, I think we can modify the way we do repacks to incorporate dynamic
> content, but I'd like to make sure that doesn't have any effects on version
> upgrades to the extension (i.e. what happens when an upgrade is installed to
> the profile directory vs. from an appdir install; will the cache files,
> regardless of whether they're installed in the appdir or profile, be copied to
> a cache location in the profile that's independent of the extension, etc.). 

Yes, the cache should live in a subdirectory of the profile directory that is independent of the install location of the extension.  In comment 2, I suggest putting them in [profile directory]/personas/cache/.


> The only other thing I'd want to make sure of would be to check the effects of
> having different files in the extensions/personas directory from the AMO
> version on an update.

I'm not sure I understand this issue; can you elaborate?

Comment 5

[Jose E. Bolanos]

Attachment: First patch

Details about the implementation:
1. The [profile]/personas/cache directory is employed as follows:

1.1 It stores a text file named personas.json, which contains the JSON string that is loaded into PersonaService.personas.
1.2 Sames goes for favorites: it stores a file named favorites.json, which contains the JSON string that is loaded into PersonaService.favorites.
1.3 Subdirectories contain the header and footer images of each cached persona, named with its correspondent id. For example, if persona with id=44 is cached, then a folder will exist in .../cache/44/ and will contain the header and footer image files.
1.4 The header and footer files are always named "header" and "footer" plus the appropriate extension per persona.

Note: This approach allows for more than one persona to be cached. However, for the time being, whenever a persona is cached all other cached personas are removed.

2. Whenever the currentPersona is set, a call to _cachePersonaImages is made. This method removes all other cached personas from the cache directory, fetches the current persona images asynchronously and stores them in the cache. The currentPersona is set whenever a persona is selected (not previewed) and when the persona data is refreshed, so it's guaranteed that the images will be updated in both cases.

3. Whenever the UI of a persona is applied (in personas.js), the code first asks if the persona is cached using the PersonaService.getCachedPersonaImages method. If the persona is cached, then it grabs the images from disk, if not then it sets them like before, from their remote location. At a normal start-up it will most likely load them from disk, since the images would have been cached during the previous session.

4. At start-up, the service loads the personas and favorites objects from disk, if available, instead of waiting for the http requests to load. However, when these finish loading the data is refreshed and the personas.json and favorites.json files are updated.

5. Added a FileUtils object (in service.js) to encapsulate disk usage.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 403873 [details] [diff] [review]
First patch

>diff -rupN client/content/personas.js client-487341/content/personas.js

>+    // First try to obtain the images from the cache
>+    let images = PersonaService.getCachedPersonaImages(persona);
>+    if (images && images.header && images.footer) {
>+      this._header.style.backgroundImage = "url(" + images.header + ")";
>+      this._footer.style.backgroundImage = "url(" + images.footer + ")";
>+    }
>+    // Else set them from their original source
>+    else {
>+      // Use the URI module to resolve the possibly relative URI to an absolute one.
>+      let headerURI = this.URI.get(persona.header,
>+                                   null,
>+                                   this.URI.get(PersonaService.baseURI));
>+      this._header.style.backgroundImage = "url(" + this._escapeURLForCSS(headerURI.spec) + ")";
>+      // Use the URI module to resolve the possibly relative URI to an absolute one.
>+      let footerURI = this.URI.get(persona.footer,
>+                                   null,
>+                                   this.URI.get(PersonaService.baseURI));
>+      this._footer.style.backgroundImage = "url(" + this._escapeURLForCSS(footerURI.spec) + ")";
>+    }

Nit: you could factor out the lines that set the backgroundImage property from the conditional blocks, simplifying this code a bit.


>diff -rupN client/modules/service.js client-487341/modules/service.js

>+    // Load cached personas data
>+    this.loadDataFromCache();
...
>     this.refreshData();

...

>+    // Load cached favorite personas
>+    this.loadFavoritesFromCache();
>     this.refreshFavorites();

Nit: if we load general and favorites data from the cache on startup, we shouldn't also refresh it immediately but rather allow the cross-session daily refresh to do so at the appropriate time.


>     // Now that we have data, pick a new random persona.  Currently, this is
>     // the only time we pick a random persona besides when the user selects
>     // the "Random From [category]" menuitem, which means the user gets a new
>     // random persona each time they start the browser.

Hmm, this is unrelated to this patch, but this comment is actually not true anymore, now that we pick new random personas periodically on a timer.


>+  /**
>+   * Obtains the cached images of the given persona. This are stored in the
>+   * _cachePersonaImages method under the directory
>+   * [profile]/personas/cache/[persona.id].
>+   * @param aPersona The persona for which to look the cached images.

Nit: to look -> to look for


>+   * @return An object with "header" and "footer" properties, each containing
>+   * the file URL of the image. Null otherwise.
>+   */
>+  getCachedPersonaImages : function(aPersona) {
>+    let cacheDirectory =
>+      FileUtils.getDirectory(FileUtils.getPersonasDirectory(), "cache");

Nit: this could be factored out into a PersonaService getter (this._cacheDirectory) to simplify the code a bit, since it gets accessed from multiple functions.


>+        let ios =
>+          Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>+
>+        let headerURI = ios.newFileURI(headerFile);
>+        let footerURI = ios.newFileURI(footerFile);

Hmm, we should add this capability to the URI module.


>+          // read and write permissions to owner and group, read-only for others.
>+          dir.create(Ci.nsIFile.DIRECTORY_TYPE, 0774);

Nit: I think the permissions would actually need to be 0775 (i.e. o+x) in order for others to read the directory, on Unix-like systems anyway.

Nit: ideally, FileUtils would be a module that provides File and Directory objects.

There aren't any major issues, though, and this patch looks great. r=myk

Comment 7

[Myk Melez [:myk] [@mykmelez]]

A few more issues to consider:

1. If we start caching multiple personas (recent, favorites), we'll need to build a cache invalidation algorithm that removes personas from the cache once they drop off one of those lists.

2. I originally thought perhaps it would make sense to store the current persona in a location on disk that matches its relative location on the server (f.e. [ProfD]/personas/cache/3/3/33/ for the Groovy Blue persona with ID 33), but this seems like unnecessary complexity (the server is optimizing for tens or hundreds of thousands of personas).

3. Another way to do this would be to cache files for the current persona in a [ProfD]/personas/cache/current/ directory.  This would obviate the need to find the right directory when removing invalid personas from the cache, but it would lead to cache file duplication if we were to later add recent/ and favorites/ directories containing recent and favorite persona files, so I think the way you're currently doing things is better.

Chris has landed the patch in changeset http://hg.mozilla.org/labs/personas/rev/53c90c4cccb0, so this bug is fixed, and any additional changes will need to take place in separate followup patches.

Comment 8

[Kev Needham [:kev]]

sorry myk, missed the update in comment #4. 

re: final issue in my comment - wanted to make sure that adding non-default files to the extension in a repack wouldn't interfere with an update that finds those non-default files in the extension directory in the profile. Storing the files outside the extension addresses that, but potentially opens other issues as we don't currently have a standard place for extras in the distribution directory, and we'd have to think that through. Don't think it's a huge deal, but definitely something we need to put some thought around.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Kev: the current implementation doesn't ship any files with the extension, so we don't need to work out these issues for the moment anyway.

Comment 10

[Jose E. Bolanos]

(In reply to comment #7)
> 2. I originally thought perhaps it would make sense to store the current
> persona in a location on disk that matches its relative location on the server
> (f.e. [ProfD]/personas/cache/3/3/33/ for the Groovy Blue persona with ID 33),
> but this seems like unnecessary complexity (the server is optimizing for tens
> or hundreds of thousands of personas).

The idea behind creating folders after the persona IDs is based on persona IDs being unique, and also to avoid having to control how to name the header and footer images (due to the different possible file extensions). If this is the case then the patch would work. Perhaps I missed your point here?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #10)
> Perhaps I missed your point here?

Yeah, my point was just that using relative file paths that matched the relative file paths of the files on the server might make it simpler to look up a persona image based on its file path.

For example, if a persona record says its header image is at 3/3/33/tbox-groovy_blue.jpg on the server, and we were to store it at [ProfD]/personas/cache/3/3/33/tbox-groovy_blue.jpg on the client, then we would just need to look it up at [ProfD]/personas/cache/[header path].

However, as I mentioned before I think this is unnecessary complexity, and it can cause problems too, since personas records don't always provide relative paths, and they might not always be relative to the same root directory, so we should probably not rely on the existing server layout for the layout of the local cache.

Comment 12

[Jose E. Bolanos]

Attachment: Patch with changes from comment #6

This patch has the changes noted by Myk in comment #6.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 404250 [details] [diff] [review]
Patch with changes from comment #6

>diff -rupN client/content/personas.js client-487341/content/personas.js

>+    let headerURL, footerURL;
>+    // First try to obtain the images from the cache
>+    let images = PersonaService.getCachedPersonaImages(persona);
>+    if (images && images.header && images.footer) {
>+      headerURL = images.header;
>+      footerURL = images.footer;

I was just thinking about this a bit more and realized you can actually simplify this further by returning an array of the two values from getCachedPersonaImages and using destructuring assignment to assign the values in the array to the two variables, i.e.:

 let [headerURL, footerURL] = PersonaService.getCachedPersonaImages(persona);

However, an even simpler version might use two methods for retrieving images, i.e. something like:

 let baseURI = this.URI.get(PersonaService.baseURI);
 let headerURL = PersonaService.getCachedHeaderURL(persona) || 
                 this.URI.get(persona.header, null, baseURI).spec;
 let footerURL = PersonaService.getCachedFooterURL(persona) || 
                 this.URI.get(persona.footer, null, baseURI).spec;


>diff -rupN client/modules/URI.js client-487341/modules/URI.js

>+/**
>+ * Gets a URI of the given nsIFile.
>+ * @param aFile The nsIFile to get a URI of.
>+ * @return The URI of the file, or null if an error occurs.
>+ */
>+URI.getFile = function(aFile) {
>+  try {
>+    return URI.ioSvc.newFileURI(aFile);
>+  }
>+  catch (ex) {
>+    return null;
>+  }
>+}

One of the goals of the JS Modules project of which this URI module is a part is to present the simplest possible API with polymorphic methods that understand different sets of parameters and branch accordingly, so we should actually overload the URI constructor and URI.get with this functionality and transparently return an nsIFileURI from those functions when they are passed an nsIFile as their first argument.

r=myk. But this can no longer be checked in as a unified patch, as Chris has already checked in the earlier version. Any changes need to be made as followup patches.

Comment 14

[Jose E. Bolanos]

(In reply to comment #13)
> However, an even simpler version might use two methods for retrieving images,
> i.e. something like:
> 
>  let baseURI = this.URI.get(PersonaService.baseURI);
>  let headerURL = PersonaService.getCachedHeaderURL(persona) || 
>                  this.URI.get(persona.header, null, baseURI).spec;
>  let footerURL = PersonaService.getCachedFooterURL(persona) || 
>                  this.URI.get(persona.footer, null, baseURI).spec;

The reason for having a single method to return both images is that several validations that pertain to both images need to be made (e.g. obtaining the directory, checking whether it exists), in order to determine whether they are cached or not. So, if we separate the method in two these validations would run twice.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #14)
> The reason for having a single method to return both images is that several
> validations that pertain to both images need to be made (e.g. obtaining the
> directory, checking whether it exists), in order to determine whether they are
> cached or not. So, if we separate the method in two these validations would run
> twice.

Sure, although given how infrequently this code gets called, I would prioritize code simplicity and readability in this case over the negligible performance benefit of running the validation code once.