1566758 - Enable telemetry for external uses of IDBFactory.open(name, options)

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task, P3)

Description

[Simon Giesecke [:sg] [he/him]]

open(name, options) is still available publicly if enabled via pref dom.indexedDB.storageOption.enabled, so we should first re-enable and evaluate the telemetry for the use of this overload, before removing/renaming it.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Previously, the telemetry only counted uses of the overload with storage set to persistent or temporary. To be sure not to break existing users by removing it, we need to also count uses like open(name, { version: 1 }) or open(name, { storage: default}).

The overload is also used when calling open(name), but such uses would use the other (portable) overload open(name, optional version) once open(name, optional options) is removed/renamed. So these uses must not be counted.

Actually, this would still miss uses that explicitly call open(name, {}), but I fear that nothing can be done about that.

So actually, this isn't about re-enabling the existing telemetry, but adding some different telemetry.

One question from my side: Do we need to distinguish sub-cases or just count uses of the overload?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

As covered somewhat in bug 1275496 it would be good to make sure we distinguish use on the web proper (http/https) from file-protocol uses and then from third-party extensions and then internal uses (system principal or if we can, principals that are from things like activity-stream/pocket/etc.)

Comment 3

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

As covered somewhat in bug 1275496 it would be good to make sure we distinguish use on the web proper (http/https) from file-protocol uses and then from third-party extensions and then internal uses (system principal or if we can, principals that are from things like activity-stream/pocket/etc.)

Can I get all required information to distinguish these four cases from the PrincipalInfo?

Comment 4

[Tom Tung [:tt, :ttung]]

(In reply to Simon Giesecke [:sg] from comment #3)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

As covered somewhat in bug 1275496 it would be good to make sure we distinguish use on the web proper (http/https) from file-protocol uses and then from third-party extensions and then internal uses (system principal or if we can, principals that are from things like activity-stream/pocket/etc.)

Can I get all required information to distinguish these four cases from the PrincipalInfo?

Yes, you are right. You might be able to distinguish them from the code path as well, though. The most straightforward way is to get the scheme object from nsIURI. And you can get the nsIURI from nsIPrincipal object (which can be transferred from PrincipalInfo). [1] is one of the examples.

[1] https://searchfox.org/mozilla-central/source/dom/indexedDB/IDBFactory.cpp#448-456

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Simon Giesecke [:sg] from comment #3)

Can I get all required information to distinguish these four cases from the PrincipalInfo?

To expand on what Tom is saying, assuming you're doing this from PBackground, I don't know that we can yet use PrincipalInfoToPrincipal, but you can certainly do approximately the same thing. Specifically, you'd sorta do what PrincipalInfoToPrincipal does at https://searchfox.org/mozilla-central/source/ipc/glue/BackgroundUtils.cpp#39 and check the type. I think the decision tree is basically:

• TSystemPrincipalInfo => "system"
• TContentPrincipalInfo: Use NS_NewURI and pull out the scheme, check the scheme (this now is allowed off the main thread):
  • "file", "http", "https" => pass-through as-is (or maybe consolidate http(s))
  • "moz-extension" => actually, yeah, maybe pass-through too? (Note that https://searchfox.org/mozilla-central/source/caps/ContentPrincipal.cpp#524 does the check here which is whether we decide to lookup the addon-policy or not"
  • "about" => probably pass this through too, as this covers most things like newtab I think?
  • anything elese => "other-content"
• TExpandedPrincipalInfo => "expanded" (which conveys a content script)
• other => "other-principal"

Maybe build/run locally and trigger the new tab pages and such, then see what about:telemetry shows you, and if it looks like they are getting categorized wrong, the specific domains might need to get mapped? (And/or just add some printfs that show the domains for local testing only so you can make sure they're not doing anything from actual http/https sites. One might use the devtools to inspect the frames in use as well, and/or to trigger indexeddb from those frames and see that the right thing happens.)

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Which is not to imply that we need to use string keys for any telemetry; those could be enumerated values/whatever makes sense. Tom is the expert for sure on telemetry on the team!

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1566758 - Removed obsolete IDB_TYPE_PERSISTENT_COUNT and IDB_TYPE_TEMPORARY_COUNT telemetry r=ttung

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1566758 - Remove obsolete telemetry definitions r=ttung

Depends on D40178

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1566758 - Added telemetry for use of IDBFactory.open(name, options) discriminated by principal types r=ttung

Depends on D40179

Comment 10

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1566758 - Replaced two uses of IDBFactory.open(name, options) by IDBFactory.open(name, version) where only version was present in options r=ttung

Depends on D40180

Comment 11

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #5)

Thanks for the detailed information on how to discriminate the principals.

Maybe build/run locally and trigger the new tab pages and such, then see what about:telemetry shows you, and if it looks like they are getting categorized wrong, the specific domains might need to get mapped? (And/or just add some printfs that show the domains for local testing only so you can make sure they're not doing anything from actual http/https sites. One might use the devtools to inspect the frames in use as well, and/or to trigger indexeddb from those frames and see that the right thing happens.)

Since we assume that this overload isn't used anymore in the wild, it will be hard to find appropriate manual test cases that use this overload. I could write some gtests for the categorization though :)

However, I don't quite understand what you mean by "specific domains might need to get mapped". The discrimination you described is based only on the scheme. Could you give an example what a wrong categorization would be?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

My general concern is that Firefox-internal/specific pages like "about:newtab" or "about:home" may sometimes be hosted on websites external to the browser even thought it doesn't necessarily look that way. (The about protocol, under the hood, is able to perform complicated and nightmarish redirections where the URI used for actual data retrieval and security purposes is not the URL displayed. The about protocol can't actually serve data itself. There can also be iframes.) In those cases, they may end up writing Firefox-specific code and I want to make sure we don't get confused by those scenarios. Onboarding tours may also do this, although I think those didn't look like "about domains".

I'm hand-waving somewhat here because I have trouble keeping up with how those pages work and what experiments may or may not be happening that impact only a portion of the user-base due to A/B tests. At least in the past, one of those pages had a preference-configurable option to load the site remotely, but I'm not sure it actually shipped. But it looked like a thing that existed when doing static inspection via searchfox, hence why I'd recommend just trying to dynamically reproduce if the need arises.

Which is perhaps to say: if we see that the options dictionary is still being used, we may want to try and reproduce locally and make sure we're not getting confused by a Firefox-specific origin. There's no need to do that as step one, just be aware of the possibility.

Comment 13

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1566758 - Fixed capitalization of boolean value

Depends on D40181

Comment 14

[Simon Giesecke [:sg] [he/him]]

Attachment: Data review request

Comment 15

[Chris H-C :chutten]

Comment on attachment 9083252 [details]
Data review request

Load balancing to Megan

Comment 16

[Megan McCorquodale]

Comment on attachment 9083252 [details]
Data review request

	1.	Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way? 
This will be documented in the probe dictionary. 

	2.	Is there a control mechanism that allows the user to turn the data collection on and off? 
Yes, by opting out of telemetry. 

	3.	If the request is for permanent data collection, is there someone who will monitor the data over time?

Not permanent data collection. 

	4.	Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1 - Technical Data. 

	5.	Is the data collection request for default-on or default-off?

Default on. 

	6.	Does the instrumentation include the addition of any new identifiers?

No new identifiers. 

	7.	Is the data collection covered by the existing Firefox privacy notice?
Yes. 

	8.	Does there need to be a check-in in the future to determine whether to renew the data? 

Check in in 6 months to determine whether to renew. 

	9.	Does the data collection use a third-party collection tool? 
No. 

-- 
Data Review +

Comment 17

[Razvan Maries]

Failed to Land: Revisions: D40178 diff 142195 ← D40179 diff 142196 ← D40180 diff 145153 ← D40181 diff 142198 ← D40424 diff 142199
Details: applying /tmp/tmpSoTY3n 1 out of 1 hunk ignored -- saving rejects to file dom/indexedDB/IDBFactory.cpp.rej abort: patch command failed: exited with status 256

Comment 18

[Simon Giesecke [:sg] [he/him]]

Rebased patches to central and resolved conflicts.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e15a767de194
Removed obsolete IDB_TYPE_PERSISTENT_COUNT and IDB_TYPE_TEMPORARY_COUNT telemetry r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/ea1ed827a2d1
Remove obsolete telemetry definitions r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/9b9af2a48cc5
Added telemetry for use of IDBFactory.open(name, options) discriminated by principal types r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/3b9d36fbce87
Replaced two uses of IDBFactory.open(name, options) by IDBFactory.open(name, version) where only version was present in options r=ttung,mythmon
https://hg.mozilla.org/integration/autoland/rev/d8d8cf09499c
Fixed capitalization of boolean value r=chutten

Comment 20

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/e15a767de194
https://hg.mozilla.org/mozilla-central/rev/ea1ed827a2d1
https://hg.mozilla.org/mozilla-central/rev/9b9af2a48cc5
https://hg.mozilla.org/mozilla-central/rev/3b9d36fbce87
https://hg.mozilla.org/mozilla-central/rev/d8d8cf09499c

Comment 21

[Simon Giesecke [:sg] [he/him]]

I just checked the data on https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2019-09-02&include_spill=0&keys=__none__!__none__!__none__&max_channel_version=nightly%252F71&measure=IDB_CUSTOM_OPEN_WITH_OPTIONS_COUNT&min_channel_version=nightly%252F68&processType=*&product=Firefox&sanitize=0&sort_by_value=0&sort_keys=submissions&start_date=2019-09-02&table=0&trim=0&use_submission_date=0

Unfortunately, contrary to our expectation, there still appear to be uses on the Web (content_http_https). What should we do about this? Extend the telemetry to identify the individual sites using this overload?

Comment 22

[Chris H-C :chutten]

Identifying sites is Category 3 data, which is not eligible for default-on data collection (like using Telemetry) in release channels. Generally the approach is to first consider if there are other approaches to accomplish this without the data. (( and then if there's no way to do so, then we proceed by bringing in Trust and thinking about ways to mitigate privacy risks to clients )).

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Suggestion from IRC in #webcompat:
12:13 PM <asuth> Does anyone know of search engines that make it feasible to find, say, uses of our overload of IDBFactory.open(DOMString name, optional IDBOpenDBOptions options = {})?
12:13 PM <asuth> We have telemetry we gathered in https://bugzilla.mozilla.org/show_bug.cgi?id=1566758 that identifies that it happens
12:39 PM <miketaylr> asuth: search engines, probably not. you could query the httparchive dataset maybe https://github.com/HTTPArchive/httparchive.org/blob/master/docs/gettingstarted_bigquery.md
12:39 PM <miketaylr> i've done similar stuff https://irccloud.mozilla.com/file/SpUO1ED8/Screen%20Shot%202019-09-03%20at%2011.39.45%20AM.png
12:40 PM <miketaylr> just a heads up, it costs about $30 to run a query -- i've burned through the $300 trial bucks they gave me

Comment 24

[Tom Tung [:tt, :ttung]]

I wonder if the case below would count to this telemetry:

indexedDB.open("test");

Simon, could you help me to verify if this is counted into this telemetry on Firefox? If so, perhaps, it's better to fitter this case out. Sorry for not catching this case in advance. (Because I think we only want to catch the number of cases for using the "options" in this telemetry, right?)

Note: I have tested that the case below would throw on chrome:

indexedDB.open("test", {version: 1});

Comment 25

[Simon Giesecke [:sg] [he/him]]

(In reply to Tom Tung [:tt, :ttung] (ooo until Aug 23rd) from comment #24)

I wonder if the case below would count to this telemetry:

indexedDB.open("test");

Simon, could you help me to verify if this is counted into this telemetry on Firefox? If so, perhaps, it's better to fitter this case out. Sorry for not catching this case in advance. (Because I think we only want to catch the number of cases for using the "options" in this telemetry, right?)

Ah, good you point that out. After looking at the patch again, I am pretty sure that indeed indexedDB.open("test") would also be counted as a use of this overload, which would explain the high use count. In the code, we cannot distinguish if someone passed no or an empty options dictionary.

This means that we need to change the webidl (https://searchfox.org/mozilla-central/source/dom/webidl/IDBFactory.webidl#36) such that the parameter is made non-optional, and we add another overload that only accepts the first parameter, so that we can properly distinguish these cases in the code.

Or do you see an easier solution?

Comment 26

[Tom Tung [:tt, :ttung]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #25)

Ah, good you point that out. After looking at the patch again, I am pretty sure that indeed indexedDB.open("test") would also be counted as a use of this overload, which would explain the high use count. In the code, we cannot distinguish if someone passed no or an empty options dictionary.

Yes, I don't think there is an existing way to distinguish that. (Though I don't think the number of users for using an empty option for indexedDB open is high)

This means that we need to change the webidl (https://searchfox.org/mozilla-central/source/dom/webidl/IDBFactory.webidl#36) such that the parameter is made non-optional, and we add another overload that only accepts the first parameter, so that we can properly distinguish these cases in the code.

This probably the best way to see the exact number of users for using the option.