Closed Bug 1566758 Opened 6 months ago Closed 5 months ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: sg, Assigned: sg)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.

Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
Blocks: 1275496

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?

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.)

Priority: -- → P3
Summary: Reenable telemetry for external uses of IDBFactory.open(name, options) → Enable telemetry for external uses of IDBFactory.open(name, options)

(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?

Flags: needinfo?(shes050117)

(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

Flags: needinfo?(shes050117)

(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.)

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!

(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?

Flags: needinfo?(bugmail)

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.

Flags: needinfo?(bugmail)
Attached file Data review request
Attachment #9083252 - Flags: data-review+
Attachment #9083252 - Flags: data-review+ → data-review?(chutten)
Comment on attachment 9083252 [details]
Data review request

Load balancing to Megan
Attachment #9083252 - Flags: data-review?(chutten) → data-review?(mmccorquodale)
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 +
Attachment #9083252 - Flags: data-review?(mmccorquodale) → data-review+
Keywords: checkin-needed

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

Flags: needinfo?(sgiesecke)

Rebased patches to central and resolved conflicts.

Flags: needinfo?(sgiesecke)

Pushed by rmaries@mozilla.com:
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

Keywords: checkin-needed
Flags: needinfo?(shes050117)

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 )).

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

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});
Flags: needinfo?(shes050117) → needinfo?(sgiesecke)

(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?

Flags: needinfo?(sgiesecke) → needinfo?(shes050117)

(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.

Flags: needinfo?(shes050117)
See Also: → 1580195
You need to log in before you can comment on or make changes to this bug.