Abstract about:home storage and make it async-ready

RESOLVED FIXED in Firefox 22

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
Firefox 22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [about-home])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
We should abastract the storage part in about:home and allow to easily replace its implementation with another async storage.
(Assignee)

Updated

6 years ago
Summary: Abstract about:home storage and make async-ready → Abstract about:home storage and make it async-ready
(Assignee)

Comment 1

6 years ago
Created attachment 691767 [details] [diff] [review]
patch v1.0

I thought about various approaches, in the end what we need is a Map, so I exposed a Map-like object that is cached asynchronously on page startup and then can be used synchronously. This required the less changes overall and is simple to use. The idea is that you always speak with the cache, so doesn't matter if the underlying storage is synchronous or not.

While working on this I figured the current snippets are manipulating localStorage directly, to show default snippets in certain cases, so I had to bump up the snippets version and exposed a showDefaultSnippets method that new versions of the snippets can use.
Not doing so means we can't replace the storage and moreover since showSnippets() loads a snippet that invoked showSnippets() we end up with a "too much recursion" error.

I wonder if Fryn may have time to look at this, feedback on the approach would be welcome regardless
Attachment #691767 - Flags: review?(fryn)
Attachment #691767 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 2

6 years ago
Les, are you still the correct person to involve for snippets version bumps?
Flags: needinfo?(lorchard)
mkelly has also been involved with snippets stuff.
Yeah, I passed the snippets torch to mkelly many months ago
Flags: needinfo?(lorchard)
Yes, I am the new snippets monkey. :D

Once this hits nightly I'll update the snippet JS to account for the new method and update any rules we have that need to be.

Comment 6

6 years ago
Comment on attachment 691767 [details] [diff] [review]
patch v1.0

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

Thanks for writing this, Marco. :)
This looks okay to me, as in it looks like it works and won't break anything as-is.

It does present the obvious inconvenience of having to replace all the localStorage IO calls in the snippet code, which could be avoided by wrapping window.localStorage in about:home with a getter and setter, but that's a bit hacky and may present problems later on.

I'm not exactly sure how this seemingly synchronous map enables us to do things asynchronously. Is only the initialization going to be async, or will reads/writes that the snippet code might do also be async and so the page's cached values may not immediately match our internally stored values?

I'd like to defer to Gavin regarding the approach, as I'm not certain that I'm seeing all the implications here.
Attachment #691767 - Flags: review?(gavin.sharp)
Attachment #691767 - Flags: review?(fyan)
Attachment #691767 - Flags: feedback?(gavin.sharp)
Attachment #691767 - Flags: feedback+
(Assignee)

Comment 7

6 years ago
(In reply to Frank Yan (:fryn) from comment #6)
> I'm not exactly sure how this seemingly synchronous map enables us to do
> things asynchronously. Is only the initialization going to be async, or will
> reads/writes that the snippet code might do also be async and so the page's
> cached values may not immediately match our internally stored values?

The cache will always be synchronous, the writes can happen asynchronously, we don't care since we talk with the cache. The snippets should never, never, never access the underlying storage directly, otherwise we are back at the start. Provided they use the abstraction we may do whatever we want with the underlying storage.
(Assignee)

Comment 8

6 years ago
notice this presumes writes are infallible, though I don't think we care for this case, since the storage can be rebuilt at any time, the data we keep here is not critical.
(In reply to Marco Bonardo [:mak] from comment #1)
> Not doing so means we can't replace the storage and moreover since
> showSnippets() loads a snippet that invoked showSnippets() we end up with a
> "too much recursion" error.

Just to make sure I understand: the "too much recursion" error was caused by the snippet trying to clear the localStorage so that the default snippet would show, but that failing because we no longer used the localStorage value (rather the cached gSnippetsMap value), and so we would try to load the same snippet again (and so on)?
Attachment #691767 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 10

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Just to make sure I understand: the "too much recursion" error was caused by
> the snippet trying to clear the localStorage so that the default snippet
> would show, but that failing because we no longer used the localStorage
> value (rather the cached gSnippetsMap value), and so we would try to load
> the same snippet again (and so on)?

yes, basically it was clearing the localStorage value and invoking showSnippets, to show the default ones.
But this patch goes through the cache and a change to the underlying storage is not replicated there, thus we kept calling showSnippets() that was calling showSnippets() and so on.
(Assignee)

Comment 11

6 years ago
Now, we need the new version of the snippets online before pushing this, or we'd end up in the too much recursion case.
I indeed noticed while testing that if I bump the snippets version I still get snippets content, I suppose from the last known version?
Hmm, the snippets service should probably be changed to fail for unknown versions, if that's not what it's doing. mkelly?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> Hmm, the snippets service should probably be changed to fail for unknown
> versions, if that's not what it's doing. mkelly?

It doesn't currently, we just set certain snippets to only go to certain versions. 

We could change it to auto-fail, or I could update the snippet JS to work in both cases (attempt to use showDefaultSnippets and fall back to the old method), which we already do for another function that was removed from about:home. This would be preferable for me, as I'm (slowly) working on a new snippets server on the side, which could have the auto-fail behavior. 

Does that work for you?
(Assignee)

Comment 14

6 years ago
(In reply to Michael Kelly [:mkelly] from comment #13)
> We could change it to auto-fail, or I could update the snippet JS to work in
> both cases (attempt to use showDefaultSnippets and fall back to the old
> method), which we already do for another function that was removed from
> about:home. This would be preferable for me, as I'm (slowly) working on a
> new snippets server on the side, which could have the auto-fail behavior. 

The best solution would be to fail with an error (4xx, 5xx). Though, looking at the code, in the error case we fallback to whatever is stored in the local cache, that is the old version of the snippets, so that wouldn't solve the problem. I'll file a separate bug to store the version of the last fetch, so on version bump we clear the storage instead of using an outdated cached version :(

I suppose at this point is better if we take your solution (check for showDefaultSnippets function) while we fix this other bug I'm going to file. The new server can have the auto-fail with an error at that point.
(Assignee)

Comment 15

6 years ago
filed bug 823547, you are cc-ed.
(Assignee)

Updated

6 years ago
Blocks: 823547
(Assignee)

Comment 16

6 years ago
After looking into bug 823547, even if I implement it (I'm mostly done), it's not enough to prevent old cached content to be used.
If the last server supported version is N, we ask for N+1 (new version of FF), and the server still returns anything, we store that anything along with version N+1 (that is what we requested). So we store snippet version N as if it was N+1.

I don't see an exit, unless the server returns an error so that the xhr fails.
(In reply to Marco Bonardo [:mak] from comment #16)
> After looking into bug 823547, even if I implement it (I'm mostly done),
> it's not enough to prevent old cached content to be used.
> If the last server supported version is N, we ask for N+1 (new version of
> FF), and the server still returns anything, we store that anything along
> with version N+1 (that is what we requested). So we store snippet version N
> as if it was N+1.
> 
> I don't see an exit, unless the server returns an error so that the xhr
> fails.

For this particular scenario that should work, though; the only thing that depends on the behavior changing is the global JS that we include with all snippets; there are no individual snippets that will break with this (and if there were we could add a rule to not serve them to the new version), so it is safe to store what the server returns once the JS is updated (which will happen tomorrow).

Hopefully by the time about:home changes version again we'll have a new snippet server that sends back errors, but if not then I can bite the bullet and add the error feature to the old server.

Comment 18

6 years ago
Is it ever necessary after this code change for showSnippets to be called more than once per page instance? If not, could we include a check inside showSnippets() to ensure that it is only called once? Alternately, we could change the function signature of showSnippets to require a parameter that loadSnippets passes. Not great for code cleanliness, but if it avoids breakage, it might be worth it.
(Assignee)

Comment 19

6 years ago
(In reply to Frank Yan (:fryn) from comment #18)
> Is it ever necessary after this code change for showSnippets to be called
> more than once per page instance? If not, could we include a check inside
> showSnippets() to ensure that it is only called once?

we may do that, yes, it could just throw if invoked a second time.
(Assignee)

Comment 20

6 years ago
Created attachment 694803 [details] [diff] [review]
patch v1.1

The tests I'm adding in bug 823547 found some error here, moerely use of "key" instead of "aKey".  I love tests.
Attachment #691767 - Attachment is obsolete: true
mak: Can you r? https://github.com/mozilla/snippets/pull/3 ? If it looks good to you, I'll merge and add it to the snippet servers.
(Assignee)

Comment 22

6 years ago
why don't you:
if (typeof showDefaultSnippets == "function") {
  showDefaultSnippets
} else {
 ...
}
(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #22)
> why don't you:
> if (typeof showDefaultSnippets == "function") {
>   showDefaultSnippets
> } else {
>  ...
> }

Updated the PR to use typeof. I also changed it around to use a polyfill instead.
(Assignee)

Comment 24

6 years ago
it looks good to me, the only thing is it won't pass a js strict rule, since in "strict" mode you can't use the function statement inside an if (http://whereswalden.com/2011/01/24/new-es5-strict-mode-requirement-function-statements-not-at-top-level-of-a-program-or-function-are-prohibited/).
Though you can do
let showDefaultSnippets = function() { ... }
there

We are not yet using strict mode in this page, but while here could be better to be ready :)
r-me with that addressed.
(Assignee)

Comment 25

6 years ago
ehr, do not use a "let" there though or will be invisible outside.
Updated PR one more time. Only thing I'm curious about is if there are any issues using `window.showDefaultSnippets`. It seems to work with the other chrome functions, but I want to double check. :D
(Assignee)

Comment 27

5 years ago
OK, I have a patch that also fixes the test, but I'm bitrotting against bug 840177, so let's wait for it.
Depends on: 840177
(Assignee)

Comment 28

5 years ago
Created attachment 713629 [details] [diff] [review]
patch v1.2

differences from v1.1:
- test rewritten, it should now setup things correctly
- added queue of callbacks to the storage init, to sync up with the test
- showSnippets() throws if invoked multiple times
Attachment #694803 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Comment on attachment 713629 [details] [diff] [review]
patch v1.2

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

Re-asking review since I had to change the above things, the idea/structure didn't change from previous r+

This can wait the merge, no big hurry.
Attachment #713629 - Flags: review?(gavin.sharp)
(Assignee)

Comment 30

5 years ago
Michael, what's the situation on your side? are the snippets updated already to call showDefaultSnippets() when it's available?
Flags: needinfo?(mkelly)
Comment on attachment 713629 [details] [diff] [review]
patch v1.2

r=me with the commented-out line in promiseBrowserAttributes removed
Attachment #713629 - Flags: review?(gavin.sharp) → review+
(In reply to Marco Bonardo [:mak] (Away 14-17 Feb) from comment #30)
> Michael, what's the situation on your side? are the snippets updated already
> to call showDefaultSnippets() when it's available?

Not yet, I was waiting for one more glance at the PR as I'm unsure if there are any issues with accessing `showDefaultSnippets` the way I did (I'm likely being too scared of messing with chrome stuff :P).

If you think it's fine I can get it on production whenever you need it up. :)
Flags: needinfo?(mkelly)
(Assignee)

Comment 33

5 years ago
(In reply to Michael Kelly [:mkelly] from comment #32)
> Not yet, I was waiting for one more glance at the PR as I'm unsure if there
> are any issues with accessing `showDefaultSnippets` the way I did (I'm
> likely being too scared of messing with chrome stuff :P).

Not sure what you mean by chrome stuff, about:home is a normal page with no privileges at all, it's mere content, so at a maximum you can break the home page (yeah, not really funny, though!).

> If you think it's fine I can get it on production whenever you need it up. :)

it looks ok to me, and I think you can send it to production any time. I'll just wait for you confirmation before pushing my patches.
JS Snippet has been updated on prod, it'll take 24-48 hours to hit everyone due to caching.
(Assignee)

Comment 35

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50b8afa272f
Target Milestone: --- → Firefox 22
(Assignee)

Comment 37

5 years ago
intermittent failure, actually :(
(Assignee)

Comment 38

5 years ago
I think I figured it as a missing setting and a couple wrong checks in the test, or at least I cannot reproduce the failure on try after these changes.
https://tbpl.mozilla.org/?tree=Try&rev=a4557e316995
https://hg.mozilla.org/mozilla-central/rev/d958753bd1a2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 41

5 years ago
Commit pushed to master at https://github.com/mozilla/snippets-service

https://github.com/mozilla/snippets-service/commit/2d3d37daad3f1bf79b807d9d3f27e36bbc58ec54
Fix bug 820834: Use gSnippetsMap over localStorage if it's available.

Tests for localStorage support and, if it fails, reverts to using the
gSnippetsMap wrapper around IndexedDB. If it passes, creates a wrapper
around localStorage that is used in place of the browser-supplied
gSnippetsMap.

We can't use the browser-supplied gSnippetsMap because it only pulls in
3 keys from localStorage, which means we can't read the geolocation data
from it.
You need to log in before you can comment on or make changes to this bug.