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

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u462496, Assigned: u462496)

Tracking

unspecified
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8710958 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Blocks: 906076
(Assignee)

Updated

2 years ago
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-
(Assignee)

Comment 2

2 years ago
Created attachment 8712856 [details] [diff] [review]
bug_1241837_browsers_to_proxy.diff

Eliminates `browsers_` field, changes `browsers` proxy to field, as Dao suggested.
Attachment #8710958 - Attachment is obsolete: true
Attachment #8712856 - Flags: review?(dao)
(Assignee)

Comment 3

2 years ago
Created attachment 8712954 [details] [diff] [review]
bug_1241837_browsers_to_proxy_2.diff

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)
(Assignee)

Comment 4

2 years ago
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-
(Assignee)

Comment 6

2 years ago
(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)
(Assignee)

Comment 8

2 years ago
> 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
(Assignee)

Comment 9

2 years ago
(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)
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
Created attachment 8713422 [details] [diff] [review]
bug_1241837_browsers_to_proxy_3.diff

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)
(Assignee)

Comment 14

2 years ago
Created attachment 8714053 [details] [diff] [review]
bug_1241837_browsers_to_proxy_4.diff

Updated patch
Attachment #8713422 - Attachment is obsolete: true
Attachment #8714053 - Flags: feedback?(dao)
(Assignee)

Comment 15

2 years ago
(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)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 17

2 years ago
Created attachment 8715237 [details] [diff] [review]
bug_1241837_browsers_to_proxy_5.diff

corrections as requested
Attachment #8714053 - Attachment is obsolete: true
Attachment #8714053 - Flags: review?(dao)
Attachment #8715237 - Flags: review?(dao)
(Assignee)

Comment 18

2 years ago
(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
(Assignee)

Comment 19

2 years ago
> 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+
(Assignee)

Comment 21

2 years ago
Created attachment 8715251 [details] [diff] [review]
bug_1241837_browsers_to_proxy_6.diff
Attachment #8715237 - Attachment is obsolete: true
Attachment #8715251 - Flags: review?(dao)

Updated

2 years ago
Attachment #8715251 - Flags: review?(dao) → review+
(Assignee)

Comment 22

2 years ago
> Looks good otherwise. Thanks!

Thank *you* :-)
(Assignee)

Comment 23

2 years ago
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
(Assignee)

Comment 28

2 years ago
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
(Assignee)

Comment 29

2 years ago
...`this` in a proxy to change scope under some circumstances?
Flags: needinfo?(dao)
(Assignee)

Comment 30

2 years ago
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)
(Assignee)

Comment 32

2 years ago
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)
(Assignee)

Comment 34

2 years ago
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).
(Assignee)

Comment 36

2 years ago
Created attachment 8716901 [details] [diff] [review]
bug_1241837_browsers_to_proxy_7.diff

added SessionStore onclose browsers fix
Attachment #8715251 - Attachment is obsolete: true
Attachment #8716901 - Flags: review?(dao)
(Assignee)

Comment 37

2 years ago
(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)
(Assignee)

Comment 39

2 years ago
Created attachment 8719642 [details] [diff] [review]
bug_1241837_browsers_to_proxy_8.diff
Attachment #8719642 - Flags: review?(dao)
(Assignee)

Updated

2 years ago
Attachment #8716901 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8719642 - Flags: review?(dao) → review+
(Assignee)

Comment 41

2 years ago
So should I ask for check-in needed at this point?
Flags: needinfo?(dao)
(Assignee)

Comment 42

2 years ago
(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)

Comment 45

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d22acf0f2bf3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.