879658 - remove non-standard APIs from FrameWorker for non-whitelisted providers

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Previously whitelisted providers (including Facebook) depend on having localStorage available, but since localStorage will never be exposed to real SharedWorkers, and we would like to move to real SharedWorkers eventually, we should avoid the compatiblity issues getting any worse by blocking access to localStorage in Workers from day one.

As an alternative, we could expose indexedDB (which will be exposed to SharedWorkers), and offer a library like https://github.com/mounirlamouri/storage.js as a simple alternative for localStorage-like needs.

Comment 1

[edA-qa mort-ora-y]

Why will localStorage not be supported in shared workers? This alone seems like a huge limitation.

As for the provider, this would create significant problem in writing a proper provider. Currently I'm using local storage for several purposes:

1. to communicate with the main (non-sidebar) document (which would of course not be necessary if it were a proper sharedworker with which the main page could communicate)

2. to track installation status of the provider (so our landing page can tell if the provider is installed)

3. to persist login credentials and check for changes in user data / error conditions

I was also planning, though not yet done, to store additional user configuration in localStorage and use it in the worker. Perhaps unlike a normal sharedWorker situation the social provider worker will be running even if there are no sidebars opened. The worker is the primary driver in the system, not the attached sidebars.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to edA-qa mort-ora-y from comment #1)
> Why will localStorage not be supported in shared workers? This alone seems
> like a huge limitation.

Mainly as it is a sync api - which is very difficult to make safe, sane and fast in a multi-threaded/multi-process world.  It may well turn out that the consensus in the standards groups and browser vendors does result in them existing, but we are trying to take a conservative approach while things are unknown.  That said though, the consensus in *our* platform group seems to be that the async indexeddb api will make it.

> 1. to communicate with the main (non-sidebar) document (which would of
> course not be necessary if it were a proper sharedworker with which the main
> page could communicate)

In a real shared worker you would have real message ports you could use, which would make some uses of this storage redundant.  We really should consider how we might expose our ports to social provider content in the shorter term (ie, even before our social implementation moves to real shared workers)

> 2. to track installation status of the provider (so our landing page can
> tell if the provider is installed)
> 
> 3. to persist login credentials and check for changes in user data / error
> conditions

IndexedDB would work there for you, right?  I agree we can't just yank localstorage from underneath providers without *something* in place for this.

> I was also planning, though not yet done, to store additional user
> configuration in localStorage and use it in the worker. Perhaps unlike a
> normal sharedWorker situation the social provider worker will be running
> even if there are no sidebars opened. The worker is the primary driver in
> the system, not the attached sidebars.

I'd guess the same basic model would still exist there in a real shared-worker world - Fx itself would arrange to keep the shared worker alive even when no content windows or sidebar are open.

Comment 3

[edA-qa mort-ora-y]

Is the indexedDb available now for the worker? If yes then I'll just switch to that now for the things that actually need storage. But will the sync API be available? There are many things which are needlessly complicated by having to use the async API.

For messaging, yes, if the main document could send/receive messages from the worker it'd remove my abuse of localStorage for that.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to edA-qa mort-ora-y from comment #3)
> Is the indexedDb available now for the worker?

Not yet - I filed bug 880741.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 6

[edA-qa mort-ora-y]

I'm not sure if being whitelisted will actually help here. The problem is that it'll be impossible to test a provider on a different domain, since it won't be whitelisted. Thus having this conditional removal will effectively be a complete removal... or can I just add my test domains to the whitelist in config?

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 760026 [details] [diff] [review]
patch

My only hesitancy here is the new param to the frameworker.  There is a part of me that feels we should just pass in the provider, in case there are any future cases where we need more info.  I also think maybe we should have provider.type and just set it in the constructor of the provider.  

The only thing I think needs to change is; Built-ins from Fx22 are removed in fx23 (other than facebook) and replaced by whitelisting, so we should probably use builtin || whitelisted for the flag. r+ with that change.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to edA-qa mort-ora-y from comment #6)
> I'm not sure if being whitelisted will actually help here. The problem is
> that it'll be impossible to test a provider on a different domain, since it
> won't be whitelisted. Thus having this conditional removal will effectively
> be a complete removal... or can I just add my test domains to the whitelist
> in config?

For testing, you can just update the whitelist pref.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> My only hesitancy here is the new param to the frameworker.  There is a part
> of me that feels we should just pass in the provider, in case there are any
> future cases where we need more info.  I also think maybe we should have
> provider.type and just set it in the constructor of the provider.  

Seems like we should just handle that when we get to those future cases.

> The only thing I think needs to change is; Built-ins from Fx22 are removed
> in fx23 (other than facebook) and replaced by whitelisting, so we should
> probably use builtin || whitelisted for the flag. r+ with that change.

Ugh, this stuff is so confusing :/ I changed the property to "blessed" rather than "builtin", and made it apply to the "whitelist" case too.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 763625 [details] [diff] [review]
updated patch

LGTM

Comment 12

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f370e521f004

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 763625 [details] [diff] [review]
updated patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 786133
User impact if declined: if we let providers use this stuff in FF23 when bug 786133 is released, we'll be constrained by compatibility issues when it comes to improving the social implementation in the future
Testing completed (on m-c, etc.): has automated tests. Could use some manual testing before we ship.
Risk to taking this patch (and alternatives if risky): pretty low risk, just changing an obvious behavior in specific cases
String or IDL/UUID changes made by this patch: none

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3ce4697e65 because I forgot to update some tests to deal with the move of getOriginActivationType.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: test fix

Comment 16

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0ceec4270e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b0ceec4270e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/104a6f247a40

Comment 19

[u279076]

Deprioritizing verification since this has in-testsuite coverage. Please drop the [qa-] tag and add the verifyme keyword if this needs QA verification.