Support arbitrary frame-nesting in Fennec session store

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: JanH, Assigned: JanH)

Tracking

(Blocks 1 bug)

Trunk
Firefox 61
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(9 attachments)

59 bytes, text/x-review-board-request
Nika
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
59 bytes, text/x-review-board-request
esawin
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
59 bytes, text/x-review-board-request
esawin
: review+
Details
59 bytes, text/x-review-board-request
snorp
: review+
Details
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

Last year
mozreview-review
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.
Attachment #8971794 - Flags: review?(mdeboer) → review-

Comment 11

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

https://reviewboard.mozilla.org/r/240538/#review246374
Attachment #8971795 - Flags: review?(mdeboer) → review+

Comment 12

Last year
mozreview-review
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.
Attachment #8971799 - Flags: review?(mdeboer) → review+

Comment 13

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

https://reviewboard.mozilla.org/r/240544/#review246392
Attachment #8971798 - Flags: review?(esawin) → review+

Comment 14

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

https://reviewboard.mozilla.org/r/240548/#review246396
Attachment #8971800 - Flags: review?(esawin) → review+
Comment on attachment 8971796 [details]
Bug 1456391 - Part 4: Use new mapFrameTree version for GeckoView.

https://reviewboard.mozilla.org/r/240540/#review246458
Attachment #8971796 - Flags: review?(snorp) → review+
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.
Attachment #8971797 - Flags: review?(snorp) → review+
Comment on attachment 8971801 [details]
Bug 1456391 - Part 9: Support PrivacyLevel form data filtering with GeckoView, too.

https://reviewboard.mozilla.org/r/240550/#review246464
Attachment #8971801 - Flags: review?(snorp) → review+
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.
Assignee

Comment 19

Last year
mozreview-review-reply
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.
Assignee

Comment 20

Last year
mozreview-review-reply
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

Last year
mozreview-review
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.
Attachment #8971793 - Flags: review?(nika) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 31

Last year
mozreview-review-reply
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

Last year
mozreview-review-reply
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

Last year
mozreview-review
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 ;-)
Attachment #8971794 - Flags: review?(mdeboer) → review+
Assignee

Comment 34

Last year
mozreview-review-reply
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

Last year
mozreview-review-reply
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?
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1458689

Comment 46

Last year
Pushed by mozilla@buttercookie.de:
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
You need to log in before you can comment on or make changes to this bug.