1241837 - Use proxy for `browsers` property in tabbrowser instead of explicit array

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[u462496]

Attachment: browsers_to_proxy.diff

Bug 906076 will make xul:browser instantiation lazy.  Presently the `browsers` property returns an explicit array of all xul:tabs' corresponding xul:browsers with corresponding indices.  The array is created upon first access to this property.

In bug 906076, attempting to access a xul:tab's xul:browser will force instantiation of the xul:browser if is isn't already.  Creating the `browsers` array as it is now would immediately instantiate all xul:browsers for all xul:tabs, defeating the optimization.  This bug uses a proxy instead to create a psuedo-array, which would only instantiate the xul:browser for the xul:tab at the index used to reference the array.

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 8710958 [details] [diff] [review]
browsers_to_proxy.diff

_browsers seems to be an unnecessary complication at this point. I think you should be able to implement the proxy like this:

<field name="browsers"><![CDATA[
  new Proxy(...)
]]></field>

Comment 2

[u462496]

Attachment: bug_1241837_browsers_to_proxy.diff

Eliminates `browsers_` field, changes `browsers` proxy to field, as Dao suggested.

Comment 3

[u462496]

Attachment: bug_1241837_browsers_to_proxy_2.diff

Updated version of previous patch, removes superfluous instances of `browsers_` cache invalidation.

Comment 4

[u462496]

An additional benefit from this bug is that the proxy would be much more efficient.  The present implementation destroys and creates a new array of xul:browsers every time tabContainer is modified in some way.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8712954 [details] [diff] [review]
bug_1241837_browsers_to_proxy_2.diff

Hmm, this doesn't seem to work as expected, e.g. gBrowser.browsers.indexOf(gBrowser.browsers[0]) returns -1. Am I missing something?

nit: please use == instead of ===

Comment 6

[u462496]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8712954 [details] [diff] [review]
> bug_1241837_browsers_to_proxy_2.diff
> 
> Hmm, this doesn't seem to work as expected, e.g.
> gBrowser.browsers.indexOf(gBrowser.browsers[0]) returns -1. Am I missing
> something?
> 
> nit: please use == instead of ===

Hmm, do I need to explicitly define the array prototype methods in the proxy?

Comment 7

[Dão Gottwald [:dao]]

I guess so... not sure how else things like indexOf could work here.

Comment 8

[u462496]

> Hmm, do I need to explicitly define the array prototype methods in the proxy?

Actually, in this case yes.  indexOf is correctly reporting because gBrowser.browsers[0] never actually gets assigned.

gBrowser.browsers[0] = gBrowser.tabs[0].linkedBrowser;
gBrowser.browsers.indexOf(gBrowser.browsers[0]);  //returns 0

Comment 9

[u462496]

(In reply to Dão Gottwald [:dao] from comment #7)
> I guess so... not sure how else things like indexOf could work here.

Well, it is a pseudo-array.  We could explicitly define read-only methods which are only few.  We shouldn't have to concern ourselves with methods that modify the array since this api is specific to tabbrowser and shouldn't be altered by external code anyway.

Comment 10

[Dão Gottwald [:dao]]

Yes, this array shouldn't be modified from the outside.

Comment 11

[u462496]

I am uploading a revised patch which includes handling for indexOf method.  I don't think any other Array.prototype accessor methods would be meaningful to this "array" in the context of its purpose here, and it is very unlikely that addons have been using them.  For example, `toString`, `join` etc have no functional purpose in an array of objects.

Really, this does not have to emulate an Array; it is a specialized map used for a specific purpose in tabbrowser.

Comment 12

[u462496]

Attachment: bug_1241837_browsers_to_proxy_3.diff

Same as previous patch, but the proxy now includes handling for indexOf method.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8713422 [details] [diff] [review]
bug_1241837_browsers_to_proxy_3.diff

(In reply to Allasso Travesser from comment #11)
> I am uploading a revised patch which includes handling for indexOf method. 
> I don't think any other Array.prototype accessor methods would be meaningful
> to this "array" in the context of its purpose here, and it is very unlikely
> that addons have been using them.  For example, `toString`, `join` etc have
> no functional purpose in an array of objects.

entries, every, filter, find, findIndex, forEach, includes, keys, map, some...? Those seem to make sense and should continue to work.

Comment 14

[u462496]

Attachment: bug_1241837_browsers_to_proxy_4.diff

Updated patch

Comment 15

[u462496]

(In reply to Dão Gottwald [:dao] from comment #13)
> entries, every, filter, find, findIndex, forEach, includes, keys, map,
> some...? Those seem to make sense and should continue to work.

A lot of the prototype methods used (index in array) test and that was fouling things up.  I wrote the proxy with "has" handler - that seems to fix everything.  Proxy is a lot simpler now.

Please tell me what you think.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8714053 [details] [diff] [review]
bug_1241837_browsers_to_proxy_4.diff

>+                if (!this.tabs[name]) { return undefined; }

Please break this up into multiple lines.

I'm sorry I'm unable to test this patch at the moment. Does this still work with this? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/@@iterator

Comment 17

[u462496]

Attachment: bug_1241837_browsers_to_proxy_5.diff

corrections as requested

Comment 18

[u462496]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 8714053 [details] [diff] [review]
> bug_1241837_browsers_to_proxy_4.diff
> 
> >+                if (!this.tabs[name]) { return undefined; }
> 
> Please break this up into multiple lines.

Ouch!

> I'm sorry I'm unable to test this patch at the moment. Does this still work
> with this?
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Array/@@iterator

Yes, I have tested successfully with all non-iterator methods on this page: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/prototype

Comment 19

[u462496]

> Yes, I have tested successfully with all non-iterator methods on this page:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Array/prototype

Ouch again, that should say "non-mutator methods".

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8715237 [details] [diff] [review]
bug_1241837_browsers_to_proxy_5.diff

>+            has: (target, name) => {
>+              if (typeof name == "string" && Number.isInteger(parseInt(name))) {
>+                return (name in this.tabs);
>+              }

please explicitly return false at the end of this method

>+            },
>+            get: (target, name) => {
>+              if (name == "length") {
>+                return this.tabs.length;
>+              }
>+              if (typeof name == "string" && Number.isInteger(parseInt(name))) {
>+                if (!this.tabs[name]) {

last nit: use 'if (!(name in this.tabs))' instead of 'if (!this.tabs[name])'

Looks good otherwise. Thanks!

Comment 21

[u462496]

Attachment: bug_1241837_browsers_to_proxy_6.diff

Comment 22

[u462496]

> Looks good otherwise. Thanks!

Thank *you* :-)

Comment 23

[u462496]

Dao, do I need to do anything now?

Comment 24

[Dão Gottwald [:dao]]

I'll land this. Normally you'd just add the checkin-needed keyword at the top of this page.

Comment 25

[Emanuel Hoogeveen [:ehoogeveen]]

Changes should also generally have a try run [1] to see if they break anything. I don't know if Dão is intending to do one, just mentioning it here since he didn't :)

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/97c27a348f09

Comment 27

[Dão Gottwald [:dao]]

Backed out (https://hg.mozilla.org/integration/fx-team/rev/3a53eb9e6554) because of lots of these failures:

chrome://browser/content/tabbrowser.xml:2830 - TypeError: this.tabs is undefined

See https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=97c27a348f09 for more

Comment 28

[u462496]

I am unable to duplicate these errors, and everything seems to work fine.  Do you think using gBrowser.tabs instead would fix this?  However, testing for `gBrowser == this` in the handler returns true for me.  Is it ever possible for `this

Comment 29

[u462496]

...`this` in a proxy to change scope under some circumstances?

Comment 30

[u462496]

I tested locally on a build from a fresh update, I am not yet confident enough to attempt to push to try server (nor even know if I should).  Would the OS make a difference?

Comment 31

[Dão Gottwald [:dao]]

The failures were OS-independent.

Have you run the failing tests locally?
https://developer.mozilla.org/en-US/docs/Browser_chrome_tests#Running_the_browser_chrome_tests

Comment 32

[u462496]

Do I need to run the entire suite of tests? (`./mach browser-chrome` is no longer valid - on my machine anyway.)  Which are the failing tests?  I am not able to discern that from the treeherder link you posted.  Do I just run `./mach mochitest browser/base/content/test/chrome/`?

Comment 33

[Dão Gottwald [:dao]]

Yeah, the docs are outdated, sorry. Use ./mach mochitest instead and then specify the test file or test directory you want to run. You can see what tests failed by clicking the orange testsuites on treeherder: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=97c27a348f09

You can also run all browser chrome tests with ./mach mochitest -f browser, but be warned that this will take a fairly long time.

Comment 34

[u462496]

I see the problem occurring when the window is closed.  This bit of code in SessionStore.jsm onclose handler says it all: 

     TabStateFlusher.flushWindow(aWindow).then(() => {
        // At this point, aWindow is closed! You should probably not try to
        // access any DOM elements from aWindow within this callback unless
        // you're holding on to them in the closure.

        // We can still access tabbrowser.browsers, thankfully.
        for (let browser of browsers) {

Don't know how to deal with this at the moment.  I'll see what I can come up with.

Comment 35

[Dão Gottwald [:dao]]

You could replace let browsers = tabbrowser.browsers with let browsers = Array.from(tabbrowser.browsers).

Comment 36

[u462496]

Attachment: bug_1241837_browsers_to_proxy_7.diff

added SessionStore onclose browsers fix

Comment 37

[u462496]

(In reply to Dão Gottwald [:dao] from comment #35)
> You could replace let browsers = tabbrowser.browsers with let browsers =
> Array.from(tabbrowser.browsers).

Yes, that works.  Thanks.

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 8716901 [details] [diff] [review]
bug_1241837_browsers_to_proxy_7.diff

>--- a/browser/components/sessionstore/SessionStore.jsm
>+++ b/browser/components/sessionstore/SessionStore.jsm
>@@ -1236,9 +1236,9 @@ var SessionStoreInternal = {
>     var tabbrowser = aWindow.gBrowser;
> 
>     // The tabbrowser binding will go away once the window is closed,
>     // so we'll hold a reference to the browsers in the closure here.
>-    let browsers = tabbrowser.browsers;
>+    let browsers = Array.from(tabbrowser.browsers);

This comment is now outdated, as well as the one you cited in comment 34.

Comment 39

[u462496]

Attachment: bug_1241837_browsers_to_proxy_8.diff

Comment 40

[Dão Gottwald [:dao]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=373d8807c0c0

Comment 41

[u462496]

So should I ask for check-in needed at this point?

Comment 42

[u462496]

(In reply to Allasso Travesser from comment #41)
> So should I ask for check-in needed at this point?

I meant, checkin-needed

Comment 43

[Dão Gottwald [:dao]]

I'm about to land this.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d22acf0f2bf3

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d22acf0f2bf3