1428320 - Ignore about: pages in storage type telemetry

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

In bug 1354500 we considered that the high amount of storage: 'persistent' usage in telemetry (see https://bugzilla.mozilla.org/show_bug.cgi?id=1354500#c4) could be because we also count about:home usage. We should ignore internal about: pages to see if that corrects the number.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1428320 - Ignore about: pages for indexedDB storage type telemetry.

Review commit: https://reviewboard.mozilla.org/r/210500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210500/

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1428320 - Extend the indexedDB storage type telemetry to version 62.

Because of a small correction in how we measure this data,
I'd like to get more releases worth of information for this.

Review commit: https://reviewboard.mozilla.org/r/210502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210502/

Comment 3

[Jan Varga [:janv]]

Comment on attachment 8940183 [details]
Bug 1428320 - Ignore about: pages for indexedDB storage type telemetry.

https://reviewboard.mozilla.org/r/210500/#review216148

::: dom/indexedDB/IDBFactory.cpp:459
(Diff revision 1)
>    if (!IsChrome() &&
>        aOptions.mStorage.WasPassed()) {
> +
> +    // Ignore internal usage on about: pages.
> +    bool isAbout = false;
> +    nsCOMPtr<nsIPrincipal> principal = PrincipalInfoToPrincipal(*mPrincipalInfo);

PrincipalInfoToPrincipal can only be called on the main thread, but the open() method can be run in workers too.

::: dom/indexedDB/IDBFactory.cpp:462
(Diff revision 1)
> +    // Ignore internal usage on about: pages.
> +    bool isAbout = false;
> +    nsCOMPtr<nsIPrincipal> principal = PrincipalInfoToPrincipal(*mPrincipalInfo);
> +    nsCOMPtr<nsIURI> uri;
> +    if (principal) {
> +      principal->GetURI(getter_AddRefs(uri));

Checking just for non null |principal| shoud be ok, but I'm not sure about ignoring nsresult returned by GetURI(). The same applies to the SchemeIs call.

Something like this shoulf be safer:
if (principal) {
  nsCOMPtr<nsIURI> uri;
  rv = principal->GetURI(getter_AddRefs(uri));
  if (NS_SUCCEEDED(rv) && uri) {
    bool isAbout;
    rv = uri->SchemeIs("about", &isAbout);
    if (NS_SUCCEEDED(rv) && isAbout) {
      ignore = true;
    }
  }
}

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8940183 [details]
Bug 1428320 - Ignore about: pages for indexedDB storage type telemetry.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210500/diff/1-2/

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8940184 [details]
Bug 1428320 - Extend the indexedDB storage type telemetry to version 62.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210502/diff/1-2/

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8940184 [details]
Bug 1428320 - Extend the indexedDB storage type telemetry to version 62.

https://reviewboard.mozilla.org/r/210502/#review216952

Approving extension of storage type probe.

Comment 7

[Marion Daly [:mdaly]]

Updates on this?

Comment 8

[Jan Varga [:janv]]

Sorry about the delay, the patch looks good. r=me

Comment 9

[Johann Hofmann [:johannh]]

Can you quickly change the flag in mozreview so that I can check this in via autoland (assuming it doesn't have conflicts)?

Thanks! :)

Comment 10

[Jan Varga [:janv]]

Comment on attachment 8940183 [details]
Bug 1428320 - Ignore about: pages for indexedDB storage type telemetry.

https://reviewboard.mozilla.org/r/210500/#review222732

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4bd8549207d
Ignore about: pages for indexedDB storage type telemetry. r=janv
https://hg.mozilla.org/integration/autoland/rev/556d77fe66b0
Extend the indexedDB storage type telemetry to version 62. r=liuche

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a4bd8549207d
https://hg.mozilla.org/mozilla-central/rev/556d77fe66b0