Closed Bug 820834 Opened 12 years ago Closed 11 years ago

Abstract about:home storage and make it async-ready

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 22

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [about-home])

Attachments

(1 file, 2 obsolete files)

patch v1.2
18.49 KB, patch
Gavin
: review+
Details | Diff | Splinter Review 
We should abastract the storage part in about:home and allow to easily replace its implementation with another async storage.
Summary: Abstract about:home storage and make async-ready → Abstract about:home storage and make it async-ready
Attached patch patch v1.0 (obsolete) — — Splinter Review 
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)
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 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+
(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.
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+
(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.
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?
(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.
filed bug 823547, you are cc-ed.
Blocks: 823547
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.
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.
(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.
Attached patch patch v1.1 (obsolete) — — Splinter Review 
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.
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.
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.
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
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
Attached patch patch v1.2 — — Splinter Review 
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
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)
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)
(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.
intermittent failure, actually :(
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
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: