Closed Bug 1241837 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

Details

Attachments

(1 file, 8 obsolete files)

Attached patch browsers_to_proxy.diff (obsolete) — Splinter Review
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.
Blocks: lazytabs
Attachment #8710958 - Flags: review?(dteller)
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>
Attachment #8710958 - Flags: review?(dteller) → review-
Eliminates `browsers_` field, changes `browsers` proxy to field, as Dao suggested.
Attachment #8710958 - Attachment is obsolete: true
Attachment #8712856 - Flags: review?(dao)
Updated version of previous patch, removes superfluous instances of `browsers_` cache invalidation.
Attachment #8712856 - Attachment is obsolete: true
Attachment #8712856 - Flags: review?(dao)
Attachment #8712954 - Flags: review?(dao)
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 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 ===
Attachment #8712954 - Flags: review?(dao) → review-
(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?
Flags: needinfo?(dao)
I guess so... not sure how else things like indexOf could work here.
Flags: needinfo?(dao)
> 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
(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.
Flags: needinfo?(dao)
Yes, this array shouldn't be modified from the outside.
Flags: needinfo?(dao)
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.
Same as previous patch, but the proxy now includes handling for indexOf method.
Attachment #8712954 - Attachment is obsolete: true
Attachment #8713422 - Flags: review?(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.
Attachment #8713422 - Flags: review?(dao)
Updated patch
Attachment #8713422 - Attachment is obsolete: true
Attachment #8714053 - Flags: feedback?(dao)
(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.
Flags: needinfo?(dao)
Attachment #8714053 - Flags: feedback?(dao) → review?(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
Flags: needinfo?(dao)
corrections as requested
Attachment #8714053 - Attachment is obsolete: true
Attachment #8714053 - Flags: review?(dao)
Attachment #8715237 - Flags: review?(dao)
(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
> 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".
Flags: needinfo?(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!
Flags: needinfo?(dao)
Attachment #8715237 - Flags: review?(dao) → review+
Attachment #8715237 - Attachment is obsolete: true
Attachment #8715251 - Flags: review?(dao)
Attachment #8715251 - Flags: review?(dao) → review+
> Looks good otherwise. Thanks!

Thank *you* :-)
Dao, do I need to do anything now?
Flags: needinfo?(dao)
I'll land this. Normally you'd just add the checkin-needed keyword at the top of this page.
Flags: needinfo?(dao)
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
Assignee: nobody → allassopraise
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
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
...`this` in a proxy to change scope under some circumstances?
Flags: needinfo?(dao)
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?
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
Flags: needinfo?(dao)
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/`?
Flags: needinfo?(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.
Flags: needinfo?(dao)
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.
You could replace let browsers = tabbrowser.browsers with let browsers = Array.from(tabbrowser.browsers).
added SessionStore onclose browsers fix
Attachment #8715251 - Attachment is obsolete: true
Attachment #8716901 - Flags: review?(dao)
(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 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.
Attachment #8716901 - Flags: review?(dao)
Attachment #8719642 - Flags: review?(dao)
Attachment #8716901 - Attachment is obsolete: true
Attachment #8719642 - Flags: review?(dao) → review+
So should I ask for check-in needed at this point?
Flags: needinfo?(dao)
(In reply to Allasso Travesser from comment #41)
> So should I ask for check-in needed at this point?

I meant, checkin-needed
I'm about to land this.
Flags: needinfo?(dao)
https://hg.mozilla.org/mozilla-central/rev/d22acf0f2bf3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.