967414 - [Datastore] navigator.getDataStores is undefined

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys, I just found trying to solve bug 967412 that in v1.3 (Gecko-2905062.Gaia-bb36b41) we are getting navigator.getDataStores as undefined:
E/GeckoConsole(  808): [JavaScript Error: "TypeError: navigator.getDataStores is not a function" {file: "app://communications.gaiamobile.org/shared/js/fb/fb_data_reader.js" line: 352}]

Any clue about this? Thanks!

Comment 1

[Germán Toro del Valle (:gtorodelvalle)]

Need-infoing Andrea about the issue :-) If you need further information, please do not hesitate to ask me about it ;-) Thanks!

Comment 2

[Andrea Marchesini [:baku]]

navigator.getDeviceStores() is exposed just to certified apps. is this the case?

Comment 3

[Germán Toro del Valle (:gtorodelvalle)]

Hi Andrea, thank you very much for your quick reply ;-)

Yeah, this code runs in the Communications app (more concretely in the Contacts sub-app, so to call it). As you can see in the logs, the concrete JavaScript file is located in the shared folder although I guess this has nothing to do with the issue ;-)

Comment 4

[Andrea Marchesini [:baku]]

Is Communications app a certified app? This is the only valid reason why getDataStores() is not exposed.
Or there is a bug in gecko :)

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Ups, sorry, I took that for granted. Yeah, the Communications app is a certified app: https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/communications/manifest.webapp#L4

So I guess the bug in Gecko option is getting stronger... :-p

Comment 6

[Boris Zbarsky [:bzbarsky]]

So just to make sure about the other conditions Gecko is checking here:

1)  "dom.datastore.enabled" is true in the builds in question?  Note that for b2g it's false ifdef RELEASE_BUILD; the question is whether the builds in question are RELEASE_BUILD.

2)  The access is being done from content code, not via an Xray?

Comment 7

[Andrew Overholt [:overholt]]

Since this seems likely to block Facebook contact importing, marking a 1.3 blocker.

Comment 8

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> So just to make sure about the other conditions Gecko is checking here:
> 
> 1)  "dom.datastore.enabled" is true in the builds in question?  Note that
> for b2g it's false ifdef RELEASE_BUILD; the question is whether the builds
> in question are RELEASE_BUILD.

Why is it only enabled for RELEASE_BUILD?  I don't remember exactly.  We should probably lift that restriction.

Comment 9

[Boris Zbarsky [:bzbarsky]]

It's _disabled_ for RELEASE_BUILD.  As for why, see bug 923417 comment 2, where you asked for this behavior?  ;)

Comment 10

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Can you answer my questions from comment 6?  Right now the bug is blocked on that information.

Comment 12

[Kai-Chih Hu [:khu]]

I don't have the answer for comment 6. I checked the codes. Yes, in b2g.js, the value of dom.datastore.enabled depends on RELEASE_BUILD. But, I don't know what the value was when reporting this issue. [e-mail], can you help? Thank you.

Comment 14

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys, sorry but I do not have the answers to comment 6. Right now Marcos (the person in charge of our builds here in Telefónica) is having a look at it and he will probably be able to answer them. Thanks!

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

We've been checking, and effectively the problem is that RELEASE_BUILD is defined even for engineering builds (and so we get:

pref("dom.datastore.enabled", false);

on the build b2g.js file).

There's an easy workaround for fixing the bugs this one block, which is to add

pref("dom.datastore.enabled", true);

to Gaia's custom-prefs.js file.

That allows fixing the other bugs, but before 1.3 is closed there should be a definite fix to enable the datastores on release builds (because otherwise there will be a lot of Gaia that will just not work).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Ehsan, what's the state of that API?

Comment 17

[Jason Smith [:jsmith]]

This isn't a build issue. We're seeing this on the Moz side as well, so I'm inclined to think the regression is in gecko.

Comment 18

[Jason Smith [:jsmith]]

Broken:

Gecko: 9731b0b7fa78

Working:

Gecko: 3d9d920ca43b

Comment 19

[Jason Smith [:jsmith]]

Weird - there's no gecko changesets in the regression range:

http://hg.mozilla.org/releases/mozilla-b2g28_v1_3/pushloghtml?fromchange=3d9d920ca43b&tochange=9731b0b7fa78

Comment 20

[Jason Smith [:jsmith]]

Busted:

Gecko: 9731b0b7fa78
Gaia: bb36b4164d3e51ca04b612e507e1178f80bf240d

Working:

Gecko: 3d9d920ca43b 
Gaia: f9a37c77efb4621a1f57e4695b497d18601fe134

Comment 21

[Jason Smith [:jsmith]]

https://github.com/mozilla-b2g/gaia/compare/f9a37c77efb4621a1f57e4695b497d18601fe134...bb36b4164d3e51ca04b612e507e1178f80bf240d

Comment 22

[Jason Smith [:jsmith]]

Weird. Absolutely nothing suspicious in the regression range. What's causing this then?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> What's causing this then?

What's causing RELEASE_BUILD to be defined?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Well, 1.3 is based on Gecko 28, right?  Gecko 28 went to beta on 2014-02-03 (the day before this bug was filed) and RELEASE_BUILD is defined on beta.  So I'm guessing that's the "regression": code depending on an API that is explicitly marked as "do not ship in a final release" and us approaching a final release...

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There are only no Gecko csets in the range because of the way pushlog works.  Your last good cset was in the same push as http://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a7e600f732f7 which is what "caused" the regression.

Comment 26

[[:fabrice] Fabrice Desré]

It seems clear to me that we rely so much on datastore in gaia now that we should just turn it on unconditionally on b2g. Any objections?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We need an answer to comment 16 first.

Comment 28

[Boris Zbarsky [:bzbarsky]]

By "unconditionally" do you include all web pages?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Oh, just certified apps.  I personally suspect that's fine, but Ehsan should weigh in.

Comment 30

[[:fabrice] Fabrice Desré]

(In reply to Boris Zbarsky [:bz] from comment #29)
> Oh, just certified apps.  I personally suspect that's fine, but Ehsan should
> weigh in.

Yep, all builds, certified apps. I don't see how that could be an issue.

Comment 31

[(no longer active)]

Making this depend on RELEASE_BUILD was a decision made before gaia started to depend on it.  I was under the impression that we lifted that restriction a while ago but it seems to not be the case.  Here is what we should do now:

Make the API available to certified apps only on all builds in b2g, and nowhere else.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: ds.patch

https://tbpl.mozilla.org/?tree=Try&rev=4995a973aac6

I want to see how B2G emulator reacts to this patch.

Comment 33

[(no longer active)]

Comment on attachment 8370883 [details] [diff] [review]
ds.patch

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

Looks good with the comment below addressed.  Over to Boris for the test_interfaces.html change.

::: dom/webidl/DataStore.webidl
@@ +5,5 @@
>   */
>  
>  typedef (DOMString or unsigned long) DataStoreKey;
>  
> +[Func="Navigator::HasDataStoreSupport",

You want this for DataStoreCursor as well.  Also please modify the tests to check that interface too.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8370883 [details] [diff] [review]
ds.patch

r=me

Comment 35

[Andrea Marchesini [:baku]]

Attachment: ds.patch

bz, I need another quick review. I changed 2 things:

1. HasDataStoreSupport enables DataStore if the callee is Chrome. This is needed because otherwise DataStoreService.js can not do: window.DataStore._create

2. interfaces.html has DataStore for b2g only. The previous patch removed these entries.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8371566 [details] [diff] [review]
ds.patch

>+  // DataStore is enabled by default for chrome code.

I believe this is not needed on trunk, since we fixed bug 965094.  I'm OK with you landing this as-is with a followup to remove on trunk, or with this landing on trunk without this bit but with this bit on branches.

And tests, of course, to make sure that it really does work.

r=me

Comment 37

[Andrea Marchesini [:baku]]

> I believe this is not needed on trunk, since we fixed bug 965094.  I'm OK
> with you landing this as-is with a followup to remove on trunk, or with this
> landing on trunk without this bit but with this bit on branches.

I file a follow up for this.

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

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f81160e35839

Comment 40

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/d9a62ccf6cc3

Comment 41

[Germán Toro del Valle (:gtorodelvalle)]

Since I did not include an STR, let me add that if you are able to synchronise with Facebook from the Contacts app and include some Facebook friends in the contact list. This bug can be considered solved and verified ;-)

Comment 42

[Loli (:lolimartinezcr)]

Tested (02/11/2014)
1.3
Gecko 06f88fa
Gaia 00cd1a

I have imported contacts from Facebook to device, and this is working