1456391 - Support arbitrary frame-nesting in Fennec session store

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Session Restore, enhancement, P1)

Description

[Jan Henning [:JanH]]

Instead of manually iterating through one level of frames, we should do the same thing that Desktop does and use something like mapFrameTree (e.g. https://dxr.mozilla.org/mozilla-central/rev/dfb15917c057f17e5143f7d7c6e1972ba53efc49/browser/components/sessionstore/content/content-sessionStore.js#487).
Maybe the extended mapFrameTree version from bug 1441810 (that optionally takes *an array* of callbacks) could be moved into the session store Utils.jsm (https://dxr.mozilla.org/mozilla-central/rev/dfb15917c057f17e5143f7d7c6e1972ba53efc49/toolkit/modules/sessionstore/Utils.jsm), so everybody (desktop, Fennec, GeckoView) can use it.

While we're at it - looking again at https://hg.mozilla.org/mozilla-central/annotate/ef8ba68c5138/mobile/android/chrome/geckoview/GeckoViewContent.js#l73, I'm not sure the copy-pasted description about handling only "non-dynamic frames" is accurate for the implementation from bug 1441810?

So maybe nSISessionStoreUtils should really be moved to toolkit so we can use "forEachNonDynamicChildFrame" [1] on Android as well.


[1] https://dxr.mozilla.org/mozilla-central/rev/dfb15917c057f17e5143f7d7c6e1972ba53efc49/browser/components/sessionstore/nsISessionStoreUtils.idl#34

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 1: Move nsISessionStoreUtils to Toolkit.

We want to use forEachNonDynamicChildFrame() on Android as well.

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

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

GeckoView has already started using a slightly modified version of mapFrameTree,
and since ssu.forEachNonDynamicChildFrame() has vastly simplified the process of
correctly using FormData/ScrollPosition.collect() for *all* (non-dynamic) child
frames, we want to use mapFrameTree for Fennec's session store as well.

Therefore, to avoid further duplication of code, we add a common version to the
session store's Utils.jsm module.
We base the code on the GeckoView implementation of mapFrameTree, which has
gained the ability to use callback *arrays*, however we still use ssu.forEach-
NonDynamicChildFrame() like Desktop currently does, instead of simply iterating
over *all* frames.

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

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 3: Use new mapFrameTree version on Desktop.

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

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 4: Use new mapFrameTree version for GeckoView.

The copy-pasted description of mapFrameTree in GeckoView already claims to only
run the callback for *non*-dynamic child frames. With this patch we make
GeckoView actually do that and unify the behaviour with Desktop.

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

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 5: Fix zoom level saving in GeckoView.

When the scroll position is "0,0" or suchlike, the scrolldata ends up as null.
We still need something to store the zoom data on, though, so we add a fallback.

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

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 6: Switch mobile session store to use defineLazyModuleGetters.

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

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 7: Move PrivacyFilter to Toolkit.

Since we want to start collecting all form data through mapFrameTree on Fennec,
too, we need to change our filtering strategy for form data.
We can no longer bail out directly during the data collection loop and instead
have to filter the data after having collected all of it.

The easiest way to do that is to start using PrivacyFilter.filterFormData() on
Android as well.

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

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 8: Use mapFrameTree in Fennec.

This brings us in line with Desktop's session store, in that we can now collect
data for arbitrarily nested frame structures.

At the same time, this also means that we no longer collect data for dynamically
added frames, so the corresponding mochitest needs to be adapted accordingly.

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

Comment 9

[Jan Henning [:JanH]]

Attachment: Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

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

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

https://reviewboard.mozilla.org/r/240536/#review246372

It's a minus only due to the number of issues I opened, not due to anything else. I expect the next version to be a rubberstamp ;)

Jan, it's great to see you back working on sessionstore stuff on Android!!

::: toolkit/modules/sessionstore/Utils.jsm:21
(Diff revision 1)
>                                     "nsISerializationHelper");
>  XPCOMUtils.defineLazyGetter(this, "SERIALIZED_SYSTEMPRINCIPAL", function() {
>    return Utils.serializePrincipal(Services.scriptSecurityManager.getSystemPrincipal());
>  });
>  
> +const ssu = Cc["@mozilla.org/browser/sessionstore/utils;1"]

Please use `XPCOMUtils.defineLazyServiceGetter()` here and move it below `serializationHelper`.

::: toolkit/modules/sessionstore/Utils.jsm:157
(Diff revision 1)
>      return null;
> +  },
> +
> +  /**
> +   * A function that will recursively call |cb| to collect data for all
> +   * non-dynamic frames in the current frame/docShell tree.

nit: please complete this JSDoc with params and return type/ value.
This is also a good place to explain the optional array of callbacks.

::: toolkit/modules/sessionstore/Utils.jsm:159
(Diff revision 1)
> +
> +  /**
> +   * A function that will recursively call |cb| to collect data for all
> +   * non-dynamic frames in the current frame/docShell tree.
> +   */
> +  mapFrameTree(frame, cb) {

Please use `mapFrameTree(frame, ...callbacks) {` and remove the array-ification line below.

::: toolkit/modules/sessionstore/Utils.jsm:168
(Diff revision 1)
> +    let children = callbacks.map(() => []);
> +
> +    // Recurse into child frames.
> +    ssu.forEachNonDynamicChildFrame(frame, (subframe, index) => {
> +      let results = this.mapFrameTree(subframe, callbacks);
> +      results.forEach((result, j) => {

Please use:
```js
for (let j = results.length - 1; j >= 0; --j) {
  if (!results[j] || !Object.getOwnPropertyNames(results[j]).length)
    continue;
  children[j][index] = result;
}
```
In other words: when I see an index used, I'd rather see the native for-loop rather than than the functional alternative. Since we're talking about tail-recursion, I'd like to make this as straightforward as possible to optimize for the JIT and avoid the function call overhead.

::: toolkit/modules/sessionstore/Utils.jsm:175
(Diff revision 1)
> +          children[j][index] = result;
> +        }
> +      });
> +    });
> +
> +    objs.forEach((obj, i) => {

Please apply the previous comment here as well.

::: toolkit/modules/sessionstore/Utils.jsm:181
(Diff revision 1)
> +      if (children[i].length) {
> +        obj.children = children[i];
> +      }
> +    });
> +
> +    let res = objs.map((obj) => Object.keys(obj).length ? obj : null);

nit: let's update this to use `Object.getOwnPropertyNames(obj).length` instead.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971795 [details]
Bug 1456391 - Part 3: Use new mapFrameTree version on Desktop.

https://reviewboard.mozilla.org/r/240538/#review246374

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971799 [details]
Bug 1456391 - Part 7: Move PrivacyFilter to Toolkit.

https://reviewboard.mozilla.org/r/240546/#review246376

/me very happy this is useful to Android now as well!! BTW, this has seen Quantum Flow perf improvements, so you're pulling in the goods stuff here.

Comment 13

[Eugen Sawin [:esawin]]

Comment on attachment 8971798 [details]
Bug 1456391 - Part 6: Switch mobile session store to use defineLazyModuleGetters.

https://reviewboard.mozilla.org/r/240544/#review246392

Comment 14

[Eugen Sawin [:esawin]]

Comment on attachment 8971800 [details]
Bug 1456391 - Part 8: Use mapFrameTree in Fennec.

https://reviewboard.mozilla.org/r/240548/#review246396

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8971796 [details]
Bug 1456391 - Part 4: Use new mapFrameTree version for GeckoView.

https://reviewboard.mozilla.org/r/240540/#review246458

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8971797 [details]
Bug 1456391 - Part 5: Fix zoom level saving in GeckoView.

https://reviewboard.mozilla.org/r/240542/#review246462

::: mobile/android/chrome/geckoview/GeckoViewContent.js:75
(Diff revision 1)
>  
>      // Save the current document resolution.
>      let zoom = { value: 1 };
>      let domWindowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>      domWindowUtils.getResolution(zoom);
> +    scrolldata = scrolldata || {};

Is scrolldata null or undefined in this case? If it's undefined, you can do 'let [formdata, scrolldata = {}]' above.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8971801 [details]
Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

https://reviewboard.mozilla.org/r/240550/#review246464

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8971801 [details]
Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

https://reviewboard.mozilla.org/r/240550/#review246466

Hmm, I think this deserves a comment in GeckoSession.saveState() since this may give an unexpected response if the session is running in private mode.

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8971801 [details]
Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

https://reviewboard.mozilla.org/r/240550/#review246466

Not quite - currently the only effecti is to allow turning form data collection off for either all or optionally only HTTPS pages, as controlled by the value of "browser.sessionstore.privacy_level". Private browsing doesn't come into this as far as I can tell.

Comment 20

[Jan Henning [:JanH]]

Comment on attachment 8971797 [details]
Bug 1456391 - Part 5: Fix zoom level saving in GeckoView.

https://reviewboard.mozilla.org/r/240542/#review246462

> Is scrolldata null or undefined in this case? If it's undefined, you can do 'let [formdata, scrolldata = {}]' above.

Let me check again, but I think it was null (and mapFrameTree explicitly returns null if it doesn't have any data available).

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8971793 [details]
Bug 1456391 - Part 1: Move nsISessionStoreUtils to Toolkit.

https://reviewboard.mozilla.org/r/240534/#review246484

::: toolkit/components/sessionstore/moz.build:11
(Diff revision 1)
> +
> +XPIDL_SOURCES += [
> +    'nsISessionStoreUtils.idl',
> +]
> +
> +XPIDL_MODULE = 'sessionstore'

It might make sense to put this into a separate xpidl module, given that it is now separte from the browser implementation.

Comment 22

[Jan Henning [:JanH]]

Comment on attachment 8971793 [details]
Bug 1456391 - Part 1: Move nsISessionStoreUtils to Toolkit.

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

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

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

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8971795 [details]
Bug 1456391 - Part 3: Use new mapFrameTree version on Desktop.

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

Comment 25

[Jan Henning [:JanH]]

Comment on attachment 8971796 [details]
Bug 1456391 - Part 4: Use new mapFrameTree version for GeckoView.

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

Comment 26

[Jan Henning [:JanH]]

Comment on attachment 8971797 [details]
Bug 1456391 - Part 5: Fix zoom level saving in GeckoView.

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

Comment 27

[Jan Henning [:JanH]]

Comment on attachment 8971798 [details]
Bug 1456391 - Part 6: Switch mobile session store to use defineLazyModuleGetters.

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

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8971799 [details]
Bug 1456391 - Part 7: Move PrivacyFilter to Toolkit.

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

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8971800 [details]
Bug 1456391 - Part 8: Use mapFrameTree in Fennec.

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

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8971801 [details]
Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

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

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

https://reviewboard.mozilla.org/r/240536/#review246372

> Please use `mapFrameTree(frame, ...callbacks) {` and remove the array-ification line below.

Hmm - for compatibility with the current users outside of GeckoView, I'd like to retain the previous behaviour and directly return the collected data object if only one callback was used, i.e. what currently happens with `return Array.isArray(cb) ? res : res[0];`.

When recursing internally within ssu.forEachNonDynamicChildFrame however, it is easier if mapFrameTree always returns an array, even it it contains only one element because we've only got one callback.
The current code achieves that by using `callbacks` when calling itself recursively, thereby forcing mapFrameTree to return an array even if there's only one callback.
While the ...-syntax is indeed nice, if I'm not mistaken it also means I'm losing the possibility of the above distinction, aren't I?

Comment 32

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

https://reviewboard.mozilla.org/r/240536/#review246372

> Hmm - for compatibility with the current users outside of GeckoView, I'd like to retain the previous behaviour and directly return the collected data object if only one callback was used, i.e. what currently happens with `return Array.isArray(cb) ? res : res[0];`.
> 
> When recursing internally within ssu.forEachNonDynamicChildFrame however, it is easier if mapFrameTree always returns an array, even it it contains only one element because we've only got one callback.
> The current code achieves that by using `callbacks` when calling itself recursively, thereby forcing mapFrameTree to return an array even if there's only one callback.
> While the ...-syntax is indeed nice, if I'm not mistaken it also means I'm losing the possibility of the above distinction, aren't I?

No, it's just a different way of writing this code, whilst keeping everything else the same.
What I meant with 'removing the arrayification' was just about the one single line below `mapFrameTree(frame, cb) {`, nothing else.

Comment 33

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

https://reviewboard.mozilla.org/r/240536/#review246734

Thanks for the updates, Jan!

::: toolkit/modules/sessionstore/Utils.jsm:168
(Diff revision 2)
> +   *                                        each non-dynamic frame in the given
> +   *                                        frame tree.
> +   * @return {object|object[]} A nested data structure containing the collected
> +   *                           data, or null if no data could be collected.
> +   *                           If an array of callbacks was used, the collected
> +   *                           data will be returned as an array likewise.

nit: s/likewise/accordingly/

::: toolkit/modules/sessionstore/Utils.jsm:198
(Diff revision 2)
> +        continue;
> +      }
> +      objs[i].children = children[i];
> +    }
> +
> +    let res =

nit: you can keep this on one line; I'm not too fussy about the 80ch limit ;-)

Comment 34

[Jan Henning [:JanH]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

https://reviewboard.mozilla.org/r/240536/#review246372

> No, it's just a different way of writing this code, whilst keeping everything else the same.
> What I meant with 'removing the arrayification' was just about the one single line below `mapFrameTree(frame, cb) {`, nothing else.

Well, if I write that function as `mapFrameTree(frame, ...callbacks) { [...]`, then `callbacks` will always be an array. So I no longer have to convert `cb` manually to an array, but at the same time my return statement at the end will no longer work as is because I no longer have access to `cb`, only to `callbacks`, which then leads to the problem described above.

Comment 35

[Mike de Boer [:mikedeboer]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

https://reviewboard.mozilla.org/r/240536/#review246372

> Well, if I write that function as `mapFrameTree(frame, ...callbacks) { [...]`, then `callbacks` will always be an array. So I no longer have to convert `cb` manually to an array, but at the same time my return statement at the end will no longer work as is because I no longer have access to `cb`, only to `callbacks`, which then leads to the problem described above.

Well, that would become `return callbacks.length == 1 ? res[0] : res;`. But I now see your point about the duality of the API and why this wouldn't work.

Alright, let's turn the wheel around and go for supporting _ONLY_ an array of callbacks and it will _ALWAYS_ return an array of results. It's up to the consumer to use the return value appropriately. This means that you'll need to fix up the desktop code to deal with this, but that change should be minimal.
What do you think?

Comment 36

[Mike de Boer [:mikedeboer]]

And instead of `callbacks` we could name them a bit more like they're telling a story, perhaps. Something like 'dataCollectors' or 'dataCollectCallbacks'? Up to you!

Comment 37

[Jan Henning [:JanH]]

Comment on attachment 8971794 [details]
Bug 1456391 - Part 2: Add mapFrameTree to sessionstore's Utils.jsm.

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

Comment 38

[Jan Henning [:JanH]]

Comment on attachment 8971795 [details]
Bug 1456391 - Part 3: Use new mapFrameTree version on Desktop.

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

Comment 39

[Jan Henning [:JanH]]

Comment on attachment 8971796 [details]
Bug 1456391 - Part 4: Use new mapFrameTree version for GeckoView.

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

Comment 40

[Jan Henning [:JanH]]

Comment on attachment 8971797 [details]
Bug 1456391 - Part 5: Fix zoom level saving in GeckoView.

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

Comment 41

[Jan Henning [:JanH]]

Comment on attachment 8971798 [details]
Bug 1456391 - Part 6: Switch mobile session store to use defineLazyModuleGetters.

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

Comment 42

[Jan Henning [:JanH]]

Comment on attachment 8971799 [details]
Bug 1456391 - Part 7: Move PrivacyFilter to Toolkit.

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

Comment 43

[Jan Henning [:JanH]]

Comment on attachment 8971800 [details]
Bug 1456391 - Part 8: Use mapFrameTree in Fennec.

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

Comment 44

[Jan Henning [:JanH]]

Comment on attachment 8971801 [details]
Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

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

Comment 45

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f82ae5c772a41f915b2a1ca68704d3d91dadf54

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1877b11a8fb6
Part 1: Move nsISessionStoreUtils to Toolkit. r=Nika
https://hg.mozilla.org/integration/autoland/rev/f7e91391e09f
Part 2: Add mapFrameTree to sessionstore's Utils.jsm. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/2f83ddb62a8c
Part 3: Use new mapFrameTree version on Desktop. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/f36be672c53c
Part 4: Use new mapFrameTree version for GeckoView. r=snorp
https://hg.mozilla.org/integration/autoland/rev/f1118e7d03ca
Part 5: Fix zoom level saving in GeckoView. r=snorp
https://hg.mozilla.org/integration/autoland/rev/7319a011fac0
Part 6: Switch mobile session store to use defineLazyModuleGetters. r=esawin
https://hg.mozilla.org/integration/autoland/rev/8f8e66175fd7
Part 7: Move PrivacyFilter to Toolkit. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/95302e1c0022
Part 8: Use mapFrameTree in Fennec. r=esawin
https://hg.mozilla.org/integration/autoland/rev/5969ba522edd
Part 9: Support PrivacyLevel form data filtering with GeckoView, too. r=snorp

Comment 47

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1877b11a8fb6
https://hg.mozilla.org/mozilla-central/rev/f7e91391e09f
https://hg.mozilla.org/mozilla-central/rev/2f83ddb62a8c
https://hg.mozilla.org/mozilla-central/rev/f36be672c53c
https://hg.mozilla.org/mozilla-central/rev/f1118e7d03ca
https://hg.mozilla.org/mozilla-central/rev/7319a011fac0
https://hg.mozilla.org/mozilla-central/rev/8f8e66175fd7
https://hg.mozilla.org/mozilla-central/rev/95302e1c0022
https://hg.mozilla.org/mozilla-central/rev/5969ba522edd