Insert tabs' linkedBrowser lazily into the document

RESOLVED FIXED in Firefox 54

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: u462496, Assigned: u462496)

Tracking

(Blocks 1 bug)

unspecified
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 47 obsolete attachments)

24.94 KB, patch
dao
: review+
Details | Diff | Splinter Review
Support bug for bug 906076.

As we begin to actually implement lazy-browser strategy, we will need to modify the code which is presently accustomed to dealing with tab.linkedBrowser, to work with lazy-browser tabs.  

However we need a fallback so if any consumers out there try to access linkedBrowser properties on a lazy-browser tab, things won't break.  This would be especially true for addons which have not yet adjusted to the lazy-browser strategy.

This bug creates a proxy for linkedBrowser which, if the tab is still in its lazy-browser state, will instantiate the browser if linkedBrowser properties are accessed.

Note that we use a proxy rather than a simple getter for linkedBrowser, because in some cases we can return some properties based on prior knowledge of the tab, and avoid linking the browser before we want to.

Note that in this bug, we are only setting up the proxy implementation, and in practice we still will not be experiencing the optimizations of lazy-browser linking, as the proxy will nearly immediately force the browser to be linked by code accessing linkedBrowser in SessionRestore.  In later incarnations of lazy-browser tabs, we will be training SessionRestore to deal with lazy-browser tabs.
Blocks: lazytabs
Priority: -- → P2
Posted patch Patch (obsolete) — Splinter Review
Attachment #8771780 - Flags: feedback?(dao+bmo)
Attachment #8771780 - Attachment is obsolete: true
Attachment #8771780 - Flags: feedback?(dao+bmo)
Depends on: 1287456
Posted patch Patch V2 (obsolete) — Splinter Review
I have split out the setting of the `hasLinkedBrowser` flag (indicating the browser has been linked to the tab) from this bug to its own bug.  This way, other bugs that depend on the flag but are otherwise independent of this bug will not be blocked by this bug.
Attachment #8772003 - Flags: feedback?(dao+bmo)
Posted patch Patch V3 (obsolete) — Splinter Review
Attachment #8772003 - Attachment is obsolete: true
Attachment #8772003 - Flags: feedback?(dao+bmo)
Attachment #8772005 - Flags: feedback?(dao+bmo)
Comment on attachment 8772005 [details] [diff] [review]
Patch V3

I don't think we need a proxy for this. Try something like this instead:

  {
    let browserParams = {
      uri: aURI,
      forceNotRemote: aForceNotRemote,
      userContextId:  aUserContextId
    };
    Object.defineProperty(t, "linkedBrowser", {
      get: function () {
        this.parentNode.tabbrowser._linkBrowserToTab(this, browserParams);
        return this.linkedBrowser;
      },
      set: function (browser) {
        delete this.linkedBrowser;
        this.linkedBrowser = browser;
      },
      configurable: true,
      enumerable: true
    });
  }
Attachment #8772005 - Flags: feedback?(dao+bmo) → feedback-
(In reply to Allasso Travesser from comment #0)
> Note that we use a proxy rather than a simple getter for linkedBrowser,
> because in some cases we can return some properties based on prior knowledge
> of the tab, and avoid linking the browser before we want to.

Missed this part. I'm not sure this is a useful optimization. What properties do you mean?
currentURI, userTypedValue, userTypedClear, audioMuted, maybe more I can't think of right now.

Probably the most useful would be currentURI.  In the original bug 906076 patch, in session restore the tab gets a property which holds a URI object generated from the url in `tabData`.  This can be passed in the proxy in place of currentURI and avoid linking the browser.  Examples of where this is useful is when the user opens Preferences page, or Addons page, every tab out there gets queried for linkedBrowser.currentURI to see if there is already a tab with those pages open.

There are gobs of call sites accessing currentURI.  In my opinion, if we can avoid modifying lots of code by simply using a proxy, we are much better off.  Also, lots of addons rely on currentURI over which we have no control.
Attachment #8772005 - Flags: feedback- → feedback?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Allasso Travesser from comment #0)
> > Note that we use a proxy rather than a simple getter for linkedBrowser,
> > because in some cases we can return some properties based on prior knowledge
> > of the tab, and avoid linking the browser before we want to.
> 
> Missed this part. I'm not sure this is a useful optimization. What
> properties do you mean?

We will be needing to encourage addon developers to sync their addons with the lazy-browser optimization.  Nevertheless, I feel we should do everything in our power to avoid losing the optimization through an addon attempting to access linkedBrowser properties.
Comment on attachment 8772005 [details] [diff] [review]
Patch V3

I think we should create the proxy in a <field/> for linkedBrowser in the tabbrowser-tab binding.
Attachment #8772005 - Flags: feedback?(dao+bmo)
How should I pass browserParams to the binding?  Set a property on the tab? eg;

in `addTab`:

    t.browserParams = browserParams;

in tabbrowser-tab binding => linkedBrowser:

    gBrowser._linkBrowserToTab(this, aURI, this.browserParams);
Flags: needinfo?(dao+bmo)
Yes, but the property should be private (_browserParams) and the URI should be part of it.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #10)
> Yes, but the property should be private (_browserParams) and the URI should
> be part of it.

Since `_browserParams` are now going to be a property of the tab, do you see any point in passing them explicitly to `_linkBrowserToTab`?  Can we just get them off the tab instead?
ie:

      <method name="_linkBrowserToTab">
        <parameter name="aTab"/>
         <body>
          <![CDATA[
            "use strict";

            let params = aTab._browserParams;
That depends on whether there will be _linkBrowserToTab call sites that could create the params object on the fly without getting it from the tab.
Posted patch Patch V4 (obsolete) — Splinter Review
I used the _linkedBrowser/linkedBrowser dance because I get recursion error if I attempt to access `this.linkedBrowser` in the proxy.  If you have a better way of dealing with this, I am all ears.
Attachment #8772005 - Attachment is obsolete: true
Attachment #8773391 - Flags: feedback?(dao+bmo)
The proxy should be overwritten by the real browser, and at that point recursion shouldn't be possible anymore.
(In reply to Dão Gottwald [:dao] from comment #15)
> The proxy should be overwritten by the real browser, and at that point
> recursion shouldn't be possible anymore.

Yes, that is the theory I thought should happen, but it isn't working that way.  That worked fine when the proxy was in `addTab`, but when it is in the binding, the recursion happens.
Flags: needinfo?(dao+bmo)
I wonder if it is a `this
I wonder if it is a `this` issue.
Dao, Here is a patch which doesn't use `_linkedBrowser` and has the recursion issue.  Can you take a look at this and see why the recursion occurs?
Attachment #8773479 - Flags: feedback?(dao+bmo)
The problem appears to only occur when explicitly forcing browser instantiation in `addTab`, for non-blank tabs,  eg "Open Link in New Tab".  For lazy tabs which get linked by the proxy, there doesn't appear to be a problem.  It also seems to be an asynchronous issue, as when I inject debug code in the proxy, the problem disappears :-/

When the proxy was in `addTab`, we either forced immediate browser instantiation, or we created the proxy, thus the proxy was never present for immediately linked tabs and there was no problem.

I'll keep looking.
I can't seem to reproduce the issue. What are the exact steps to do so?
Flags: needinfo?(dao+bmo)
What I did:

Restore to a session of 5 tabs (Wikipedia pages).
On one of the tab, right-click on link, click "Open Link in New Tab".

The problem is, as I said, it doesn't always occur.  I can stop it from occurring just by injecting some debug code in the proxy.  I think there may be a race issue.  So on different setups things may be different.  I'm on Mac OS X.
...on mine, after clicking "Open Link in New Tab", a new tab appears, the throbber runs with label saying "connecting".  Throbber stops, label will say either "connecting" or "New Tab".  I am unable to switch to the tab.

I get this error:
JavaScript error: chrome://browser/content/tabbrowser.xml, line 6385: too much recursion
Posted patch patch v5 (obsolete) — Splinter Review
It's probably due to XBL weirdness... bindings being constructed at unexpected times and fields being evaluated lazily make this whole setup somewhat unpredictable.

I moved the proxy creation back to the tabbrowser binding and did some additional cleanup.
Attachment #8773391 - Attachment is obsolete: true
Attachment #8773479 - Attachment is obsolete: true
Attachment #8773391 - Flags: feedback?(dao+bmo)
Attachment #8773479 - Flags: feedback?(dao+bmo)
Attachment #8773708 - Flags: review?(allassopraise)
No longer depends on: 1287456
Comment on attachment 8773708 [details] [diff] [review]
patch v5

Review of attachment 8773708 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but we need to have the test in the proxy for both the getter and setter:

                  if (!t.hasBrowser) {
                    this._linkBrowserToTab(t, aURI, browserParams);
                  }

Otherwise, this condition can occur:

  function(aTab) {
    let browser = aTab.linkedBrowser;                         // browser holds reference to the proxy
    browser.addEventListener("SwapDocShells", this);          // proxy runs `gBrowser._linkBrowserToTab`,
                                                              // `aTab.linkedBrowser` now references real browser,
                                                              // but `browser` still references proxy
    browser.addEventListener("oop-browser-crashed", this);    // proxy runs `gBrowser._linkBrowserToTab` again,
                                                              // because `browser` is still referenced to proxy.
  },
Attachment #8773708 - Flags: review?(allassopraise) → review-
Ugh, too bad that the proxy can be kept alive like that.

But even with that modification, there's still something else going on. browser-chrome tests don't seem to work at all. I'm getting this error: TypeError: this.mCurrentBrowser.stop is not a function
(In reply to Dão Gottwald [:dao] from comment #26)
> But even with that modification, there's still something else going on.
> browser-chrome tests don't seem to work at all. I'm getting this error:
> TypeError: this.mCurrentBrowser.stop is not a function

Which tests in particular?
./mach mochitest --browser-chrome browser/base/content/test/general/ is all I did.
(In reply to Dão Gottwald [:dao] from comment #26)
> Ugh, too bad that the proxy can be kept alive like that.

Is there a way that from inside the proxy, we can get a reference to the caller?

ie,

[tab.linkedBrowser].audiomuted

inside the proxy we get the reference to the [tab.linkedBrowser].

If so, then we could set that reference to the real browser once the browser was linked.  Then the proxy wouldn't need to do anything else.
Also:

+                switch (key) {
+                  case "currentURI":
+                    return aBrowserParams.uri;
+                }

`aBrowserParams.uri` should be converted to a nsIURI object.

But IAE, `aBrowserParams.uri` will either be "about:blank" or `undefined`, which wouldn't be very meaningful to consumers.  When we modify SessionStore code, we will be assigning a property to the lazy tab which will hold the url from the restore data, ie, the url to which the tab will be restored, which will be more meaningful.

So I think we should wait until those modifications are made, and just leave it off for now.
Posted patch patch v6 (obsolete) — Splinter Review
(In reply to Allasso Travesser from comment #30)
> Also:
> 
> +                switch (key) {
> +                  case "currentURI":
> +                    return aBrowserParams.uri;
> +                }
> 
> `aBrowserParams.uri` should be converted to a nsIURI object.

I didn't mean to leave this part in the patch and had already removed it locally. Here's the current version.
Attachment #8773708 - Attachment is obsolete: true
(In reply to Allasso Travesser from comment #29)
> (In reply to Dão Gottwald [:dao] from comment #26)
> > Ugh, too bad that the proxy can be kept alive like that.
> 
> Is there a way that from inside the proxy, we can get a reference to the
> caller?
> 
> ie,
> 
> [tab.linkedBrowser].audiomuted
> 
> inside the proxy we get the reference to the [tab.linkedBrowser].
> 
> If so, then we could set that reference to the real browser once the browser
> was linked.  Then the proxy wouldn't need to do anything else.

No, I don't think this can work. We'd need to use defineProperty for that like I did in comment 4.
Posted patch Patch V7 (obsolete) — Splinter Review
Attachment #8774129 - Flags: review?(dao+bmo)
> But even with that modification, there's still something else going on.
> browser-chrome tests don't seem to work at all. I'm getting this error:
> TypeError: this.mCurrentBrowser.stop is not a function

The problem was due to the fact that `mCurrentBrowser` was not being updated properly.  This was because the `select` event dispatched from tabbox.xml code which triggers `updateCurrentBrowser` was not being dispatched due to a tab not having a browser.

I have updated the patch to accommodate this.  This includes the addition of public method tabbrowser.ensureTabHasBrowser().

Actually, all of these changes were part of the original patch and would have been implemented anyway.  In part, this is the mechanism that makes the tab browser-ready when the tab is selected, either by user action or programatically.
I have run `./mach mochitest --browser-chrome browser/base/content/test/general/` and had no errors, other than errors which also exist when running unmodified build.
Comment on attachment 8774129 [details] [diff] [review]
Patch V7

tabbox.xml shouldn't know about tabbrowser.

I think we should probably override the getRelatedElement method in the tabbrowser-tabs binding instead.
Attachment #8774129 - Flags: review?(dao+bmo) → review-
Attachment #8773739 - Attachment is obsolete: true
Posted patch Patch V8 (obsolete) — Splinter Review
Updated patch implementing Dao's suggestions.  I made some assumptions about tabbrowser-tab that I felt were safe to clean up the code in the override.  Not sure if the test for aTab is still needed though.
Attachment #8774129 - Attachment is obsolete: true
Attachment #8774344 - Flags: feedback?(dao+bmo)
Comment on attachment 8774344 [details] [diff] [review]
Patch V8

getRelatedElement should just call _linkBrowserToTab directly instead of ensureTabHasBrowser. No need to extend the API surface for this.

browserParams should be either a property on the tab (as _browserParams) or an argument, not sometimes this and sometimes that.
Attachment #8774344 - Flags: feedback?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #38)
> Comment on attachment 8774344 [details] [diff] [review]
> Patch V8
> 
> getRelatedElement should just call _linkBrowserToTab directly instead of
> ensureTabHasBrowser. No need to extend the API surface for this.

Eventually we will need some public method for others to be able to link the browser (eg, in SessionStore) whatever we call it or however it is handled.  This seemed like a good place to introduce that.

Also, is it proper for a method in `tabbrowser-tabs` binding to call a private method in `tabbrowser`?
(In reply to Allasso Travesser from comment #39)
> (In reply to Dão Gottwald [:dao] from comment #38)
> > Comment on attachment 8774344 [details] [diff] [review]
> > Patch V8
> > 
> > getRelatedElement should just call _linkBrowserToTab directly instead of
> > ensureTabHasBrowser. No need to extend the API surface for this.
> 
> Eventually we will need some public method for others to be able to link the
> browser (eg, in SessionStore) whatever we call it or however it is handled. 
> This seemed like a good place to introduce that.

Let's worry about that when we're there.

> Also, is it proper for a method in `tabbrowser-tabs` binding to call a
> private method in `tabbrowser`?

Yes, that's fine, the two bindings belong closely together.
Posted patch Patch V9 (obsolete) — Splinter Review
Attachment #8774344 - Attachment is obsolete: true
Attachment #8774435 - Flags: review?(dao+bmo)
Comment on attachment 8774435 [details] [diff] [review]
Patch V9

>       <method name="_linkBrowserToTab">
>         <parameter name="aTab"/>
>-        <parameter name="aURI"/>
>-        <parameter name="aParams"/>
>         <body>
>           <![CDATA[
>             "use strict";
>
>-            // Supported parameters:
>-            // forceNotRemote, userContextId
>-
>-            let uriIsAboutBlank = !aURI || aURI == "about:blank";
>+            let { uri, forceNotRemote, userContextId } = aTab._browserParams || {};

Why || {}?

We should also delete aTab._browserParams when it's not needed anymore.

>+      <method name="_createBrowserProxy">
>+        <parameter name="aTab"/>
>+        <parameter name="aBrowserParams"/>

aBrowserParams is unused
Posted patch Patch V10 (obsolete) — Splinter Review
Attachment #8774435 - Attachment is obsolete: true
Attachment #8774435 - Flags: review?(dao+bmo)
Attachment #8774449 - Flags: review?(dao+bmo)
Comment on attachment 8774449 [details] [diff] [review]
Patch V10

>             let browserParams = {
>+              uri: aURI,
>               forceNotRemote: aForceNotRemote,
>               userContextId:  aUserContextId
>             };
>+            t._browserParams = browserParams;

nit: just use t._browserParams = {...}; and get rid of the browserParams variable.

>+      <method name="getRelatedElement">
>+        <parameter name="aTab"/>
>+        <body>
>+        <![CDATA[
>+          if (!aTab)
>+            return null;
>+          // Tab must have a browser.  This also serves to make the tab
>+          // browser-ready when the tab is selected if tab is currently
>+          // in lazy-browser state.
>+          if (!aTab.hasBrowser) {
>+            this.tabbrowser._linkBrowserToTab(aTab);
>+          }
>+          return this.ownerDocument.getElementById(aTab.linkedPanel);

You can just use 'document' here.

Thanks!
Attachment #8774449 - Flags: review?(dao+bmo) → review+
Posted patch Patch V11 (obsolete) — Splinter Review
Here you go :-)
Attachment #8774449 - Attachment is obsolete: true
I'm thinking it may be better to land the patches in bug 1289370, and get all the call sites taken care of first.  Things may look a lot better then.
This patch causing failures probably means that there's something wrong with the patch. It shouldn't need bug 1289370 to make the failures go away.
(In reply to Dão Gottwald [:dao] from comment #46)
> Lots of failures:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ee950aa12c

In browser_aoutofocus_background.js, the problem occurs because in the test, a promise is created in BrowserTestUtils.browserLoaded while tab.linkedBrowser is a proxy.  Even though the real browser gets instantiated in the promise, the `browser` variable is still referencing the proxy (just like in comment 25).  So the test `msg.target == browser` (real_browser == fake_browser) returns false, and the resolve code is never called, so the test finally times out.

browser_aoutofocus_background.js:
https://dxr.mozilla.org/mozilla-central/source/dom/tests/browser/browser_autofocus_background.js#15

  let loadedPromise = BrowserTestUtils.browserLoaded(tabs[1].linkedBrowser);

https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#183
BrowserTestUtils : {
  
  ...

  browserLoaded(browser, includeSubFrames=false, wantLoad=null) {
    function isWanted(url) {
      if (!wantLoad) {
        return true;
      } else if (typeof(wantLoad) == "function") {
        return wantLoad(url);
      } else {
        // It's a string.
        return wantLoad == url;
      }
    }

    return new Promise(resolve => {
      let mm = browser.ownerDocument.defaultView.messageManager;
      mm.addMessageListener("browser-test-utils:loadEvent", function onLoad(msg) {
--->    if (msg.target == browser && (!msg.data.subframe || includeSubFrames) &&
            isWanted(msg.data.url)) {
          mm.removeMessageListener("browser-test-utils:loadEvent", onLoad);
          resolve(msg.data.url);
        }
      });
    });
  },

As an experiment, I added to linkedBrowser proxy to return tab.permanentKey for browser.permanentKey.  I then changed the test in BrowserTestUtils.browserLoaded to compare permanentKey instead:

        if (msg.target.permanentKey == browser.permanentKey && (!msg.data.subframe || includeSubFrames) &&
            isWanted(msg.data.url)) {

The promise resolves, and the test runs without failures.

I suspect other failures are due to this kind of issue as well, and I will be examining each one.

I am wondering, we will need to be "training" Firefox to deal with lazy-browser tabs.  Will we need to be doing something similar with our tests as well?
Hmm, sounds like this might break many consumers that keep a reference to the browser. Maybe we should make linkedBrowser a smart getter rather than a proxy after all. Would that greatly reduce the effectiveness of bug 906076?
(In reply to Dão Gottwald [:dao] from comment #50)
> Hmm, sounds like this might break many consumers that keep a reference to
> the browser. Maybe we should make linkedBrowser a smart getter rather than a
> proxy after all. Would that greatly reduce the effectiveness of bug 906076?

To answer your question, in vanilla Firefox, no, it would not reduce its effectiveness because theoretically we can modify all consumers to deal with lazy-browser tabs properly.  It is when you bring addons into the picture where yes, it could greatly reduce its effectiveness.

There is more to consider here also, I believe.

If we go with the getter, in Firefox code we would have to make sure that we modify every single consumer that does anything with linkedBrowser.  We would have to modify code in some places where we otherwise would not have had to because the proxy could have substituted an appropriate value.  We could consider how extensive the additional code modification would be without the proxy, versus the extent of the additional modification that would result from using it.

In other words, we are either having to modify additional code which we wouldn't have to had we used the proxy, or we are having to modify additional code at those sites which hold a reference to the proxy in a way which causes things to break.  Regarding the latter, In comment 49 I showed how we could fix it just by comparing permanentKey's instead of the browsers.  In other cases it would just be a matter of making sure we instantiate the browser explicitly when we know we are going into a situation where the reference will be a problem.

My inclination is toward keeping the proxy if we can, and to investigate more closely the cost of each method before just assuming that there are tons of situations out there where holding a reference is a problem.  Because the proxy at least gives us some defense against non-compliant addons.  The getter would give us no defense at all (unless there is a way to write it of which I am unaware).  One ugly scenario I can think of is if an addon polls each tab for its url some time after startup on a high-volume session.  All of the delay which was saved from startup now suddenly occurs at some point mid-session and the user doesn't know why, they just experience Firefox freezing up for several seconds (or 30 or 40) for some apparently unknown reason.  And then they have also just lost all of the memory optimization.

Of course the third option is to not have either a smart getter or a proxy and to send out depracation warnings to addon devs telling them that if they don't comply by such and such a version their addon is going to break :-)  To be honest that is something that I am not entirely against.  Seems like Australis was kinda that way, and we survived :-)
My opinion is to go all-out with this thing to make it as robust as possible.  I have done a lot of benchmarking on this which shows there is a LOT to be gained in resource and performance optimization.  Yes, it is very radical and invasive, but let's bite the bullet and do what it takes.  There may be some growing pains but we can do what we can to minimize that as much as possible.  But once we get through this we will have something clean and powerful, and AFAIK, something no one else is doing.
BTW, I don't know how we could deal with `browsers` in the way we talked about in bug 1289370 without using the proxy.  If we use just a getter, all of the places where we iterate over `browsers` would instantiate all of the browsers.  We would have to iterate over tabs instead.
I examined all of the failed tests (65 unique) reported in https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ee950aa12c&selectedJob=24520401.  Most of them run successfully if I change one line of code in BrowserTestUtils.jsm:

65 tests total
53 have no failures with the modification in BrowserTestUtils.jsm
12 still have failures - 7 if these have open bugs filed against them.

The change in BrowserTestUtils.jsm is the change I referred to in comment 49, comparing permanentKeys instead of browser in the test in BrowserTestUtils.browserLoaded().  This would be one way of dealing with the stale reference issue in that method, though it could be dealt with differently.

I will be examining the remaining 12 tests which still have failures.
(In reply to Allasso Travesser from comment #51)
> My inclination is toward keeping the proxy if we can, and to investigate
> more closely the cost of each method before just assuming that there are
> tons of situations out there where holding a reference is a problem.

I'm not assuming that there are "tons of situations", but enough to worry about. Holding a reference to a browser (like any other DOM node) is a very standard and reasonable thing to do and breaking that will be surprising to say the least.

> Of course the third option is to not have either a smart getter or a proxy
> and to send out depracation warnings to addon devs telling them that if they
> don't comply by such and such a version their addon is going to break :-) 
> To be honest that is something that I am not entirely against.  Seems like
> Australis was kinda that way, and we survived :-)

The cost / benefit ratio is different than with Australis, so we're not going to do that.

(In reply to Allasso Travesser from comment #54)
> I examined all of the failed tests (65 unique) reported in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=36ee950aa12c&selectedJob=24520401.  Most of them run
> successfully if I change one line of code in BrowserTestUtils.jsm:
> 
> 65 tests total
> 53 have no failures with the modification in BrowserTestUtils.jsm
> 12 still have failures - 7 if these have open bugs filed against them.
> 
> The change in BrowserTestUtils.jsm is the change I referred to in comment
> 49, comparing permanentKeys instead of browser in the test in
> BrowserTestUtils.browserLoaded().  This would be one way of dealing with the
> stale reference issue in that method, though it could be dealt with
> differently.
> 
> I will be examining the remaining 12 tests which still have failures.

Okay, I'm eager to see what kind of issues the remaining 12 tests have.
(In reply to Dão Gottwald [:dao] from comment #55)
> The cost / benefit ratio is different than with Australis, so we're not
> going to do that.

Oh, I didn't really think we ever would :-)  I'm sorry, that was a bit tongue-in-cheek.

> Okay, I'm eager to see what kind of issues the remaining 12 tests have.

Okay,  I'm on it now.

BTW, sorry I might have been a bit passionate about this, but to be clear, I am very happy to defer to your wisdom on all of this.
Remaining 12 tests diagnoses:

Here are the 8 more obvious ones - the remaining 4 I will continue to work on.
They are grouped into categories:

--------------------------------------------------

problem: holding a reference that continues to point to the proxy after the
browser is linked.
Fixed by forcing browser instantiation just prior to obtaining the reference.
(this is not suggesting the ultimate fix for this problem, but only to
 demonstrate what the problem is)
---

browser_bug462673.js:
browser/base/content/test/general/browser_bug462673.js
line 24
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug462673.js#24

browser_739805.js:
browser/components/sessionstore/test/browser_739805.js
line 20
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_739805.js#20

browser_contextmenu_childprocess.js
browser/base/content/test/general/browser_contextmenu_childprocess.js
line 8
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_contextmenu_childprocess.js#8

browser_alltabslistener.js
browser/base/content/test/general/browser_alltabslistener.js
line 92, 93
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_alltabslistener.js#92

browser_tabfocus.js
browser/base/content/test/general/browser_tabfocus.js
line 118, 121
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_tabfocus.js#118

--------------------------------------------------

problem: tab.linkedBrowser.addEventListener() fails to run properly.
Seems like it is a problem with the proxy, since the proxy should return
addEventListener() as a function in the real browser's scope (`this`).
I will investigate further.
---

browser_passwordmgr_switchtab.js:
toolkit/components/passwordmgr/test/browser/browser_passwordmgr_switchtab.js
line 38
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_passwordmgr_switchtab.js#38

browser_bug817947.js
browser/base/content/test/general/browser_bug817947.j
line 51
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug817947.js#51

--------------------------------------------------

Item which gave the same test results when I tested on default build:
---

browser_mcb_redirect.js    (treeherder references this bug 1288001)

--------------------------------------------------
More reports:

browser/components/sessionstore/test/browser_purge_shistory.js
Results match default

browser/base/content/test/urlbar/browser_tabMatchesInAwesomebar.js
Apparently browser_tabMatchesInAwesomebar.js was re-written very recently.  After I updated to the new version, there were no more failures.

This leaves:
browser/components/sessionstore/test/browser_history_persist.js
browser/components/sessionstore/test/browser_crashedTabs.js

These two also appear to have the held reference issue.  Instantiating the browser before referencing at each point does remove the failures, however there are several sites where this occurs.  I feel there is a more elegant solution somewhere for this and I am working on finding it.
browser_crashedTabs.js
browser/components/sessionstore/test/browser_crashedTabs.js
Realized it was working intermittently.  Bugs have been filed on this behavior: bug 1242115 bug 1251425
(Though this still needs the patch in BrowserTestUtils.jsm that fixes all the others)

browser_history_persist.js
browser/components/sessionstore/test/browser_history_persist.js
lines 27, 35, 65, 73
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_history_persist.js#27
This seems like a problem with the proxy.  `let sessionHistory = browser.sessionHistory;` should return the real browser's sessionHistory object, which it does according to the the stack trace.  However it generates an error:
25 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_history_persist.js | Uncaught exception - [Exception... "Method not implemented'Method not implemented' when calling method: [nsIWebNavigation::sessionHistory]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: get_sessionHistory :: line 454"  data: no]
Stack trace:
    get_sessionHistory@chrome://global/content/bindings/browser.xml:454:1
    _createBrowserProxy/aTab.linkedBrowser<.get@chrome://browser/content/tabbrowser.xml:2130:21
    check_history_not_persisted@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_history_persist.js:27:7

I'll look into that.  The problem may be related to the problem in browser_passwordmgr_switchtab.js and browser_bug817947.js (see comment 57)

So everything is accounted for now except those three.
Remaining tests which still need investigation, apparently having a problem with the proxy:

browser_history_persist.js
browser_passwordmgr_switchtab.js
browser_bug817947.js
I have been working on a couple of variations on the browser proxy scheme to deal with the referencing problem within the lazy browser API itself.

In the first one, rather than pointing linkedBrowser to the real browser once it is instantiated, I have tried creating the proxy using a real browser element as the target.  Then when it comes time to instantiate the browser, `linkedBrowser` *continues* to point to the proxy. It works something like this:

let browser = document.createElementNS(NS_XUL, "browser");

aTab.linkedBrowser = new Proxy(browser, {
  get: (target, key) => {
    if (!aTab.hasBrowser) {
      this._linkBrowserToTab(aTab);
    }

    let value = target[key];
    if (typeof value == "function") {
      return value.bind(target);
    } else {
      return value;
    }
  },
  set: (target, key, value) => {
    if (!aTab.hasBrowser) {
      this._linkBrowserToTab(aTab);
    }
    target[key] = value;
  }
});

When it comes time to escalate the tab to its full tabbrowser-tab functionality, the real browser element is passed through `_linkBrowserToTab` to `_createBrowser`, and is used for the browser element.

Consumers continue to access browser properties through the proxy, even after the browser has been "instantiated", that is, bound to XBL and all the other machinations done in `_linkBrowserToTab` and `_createBrowser`.

There is virtually no overhead for creating the browser element, and thus no loss in the optimization promised by lazy-browser API.

However I am having a little difficulty with this as passing the "browser" (which is actually the proxy) to some functions breaks.  I will spend more time experimenting with this, but I am thinking that proxies may have some limitations and may not appear exactly as the target element is all respects.
In the second approach, there is no proxy at all.  Upon tab creation, there is also a real browser element created, even if the tab will be lazy, and that real browser is immediately linked to the tab.  This browser will always be the linkedBrowser for that tab.

If the tab is lazy, properties are defined on the browser to stand in for the properties which would normally appear when the the browser is added to the DOM and is XBL bound.  Getters and setters for these properties will fully instantiate the browser (add browser to the DOM and all the other machinations done in `_linkBrowserToTab` and `_createBrowser`),

When a fullbut not before first deleting all of the "fake" properties.  These properties will be replaced by those in the XBL binding.

When it comes time to fully instantiate the browser (either explicitly or by a consumer attempting to access an XBL property, first all of the "fake" XBL properties are deleted. Then the browser is passed through `_linkBrowserToTab` to `_createBrowser`, and is used for the browser element. (like the example in comment 61).  Here the browser will be bound and all of the real XBL properties and methods will become available.

So consumers are always dealing with a real browser, and the same object from beginning to end.  Thus there are no problems with held references.  And because they are dealing with a real browser element, many of the properties accessed or functions called on the browser work normally and we are not forced to fully instantiate the browser if it is not needed to fulfill a request.  eg, browser.addEventListener, even on a lazy browser, adds an event listener to the browser and it will work as it always should, whether it is fully instantiated or not.  No hocus pocus needed.  You also no longer have holes in `_tabForBrowser`.  Iteration over `browsers` can be handled more gracefully as well.

As stated in comment 61, there is virtually no overhead for creating the browser element, and thus no loss in the optimization promised by lazy-browser API.

I worked up a patch for this strategy.  It is a little bit kludgy, but it seems to work very well.  All of the tests that failed previously (comment 46) with the proxy, now all pass with this strategy, with no modification required to the tests or test API.  It just seems to be a lot more stable.  Maybe if you examine the patch you will be inspired to some ideas to make it more elegant.

I will upload the patch.
Posted patch Patch V12 (obsolete) — Splinter Review
Attachment #8776828 - Flags: feedback?(dao+bmo)
Comment on attachment 8776828 [details] [diff] [review]
Patch V12

That's an interesting idea. So the only expensive part is inserting a browser into the document, right? If so, you should just call _createBrowser right away instead of what you're doing in _createLazyBrowser.

I wonder which browser properties should actually cause the browser to be inserted into the document. Doing it for all properties seems excessive and might be unnecessary.
Attachment #8776828 - Flags: feedback?(dao+bmo) → feedback+
(In reply to Dão Gottwald [:dao] from comment #64)
> Comment on attachment 8776828 [details] [diff] [review]
> Patch V12
> 
> That's an interesting idea. So the only expensive part is inserting a
> browser into the document, right? If so, you should just call _createBrowser
> right away instead of what you're doing in _createLazyBrowser.

Well you're right.  I was assuming creating the additional elements in _createBrowser would be expensive but it's really nothing, about 1/10 the time it takes to add all the getters.

As far as optimization goes, on my machine, using _createBrowser, and even adding the full suite of getters, it looks like about 0.6 ms per browser.  So yes, inserting it into the DOM is where all the cost is.

> I wonder which browser properties should actually cause the browser to be
> inserted into the document. Doing it for all properties seems excessive and
> might be unnecessary.

I agree.  I'll see if I can come up with a list of reasonable properties.  If we do it that way, we can also eliminate _createBrowserPropertyNamesList called by the constructor.
(In reply to Allasso Travesser from comment #65)
> > I wonder which browser properties should actually cause the browser to be
> > inserted into the document. Doing it for all properties seems excessive and
> > might be unnecessary.
> 
> I agree.  I'll see if I can come up with a list of reasonable properties. 
> If we do it that way, we can also eliminate _createBrowserPropertyNamesList
> called by the constructor.

It wouldn't surprised me if getRelatedElement is the only place where we need to insert the browser...
(In reply to Dão Gottwald [:dao] from comment #66)
> (In reply to Allasso Travesser from comment #65)
> > > I wonder which browser properties should actually cause the browser to be
> > > inserted into the document. Doing it for all properties seems excessive and
> > > might be unnecessary.
> > 
> > I agree.  I'll see if I can come up with a list of reasonable properties. 
> > If we do it that way, we can also eliminate _createBrowserPropertyNamesList
> > called by the constructor.
> 
> It wouldn't surprised me if getRelatedElement is the only place where we
> need to insert the browser...

If you mean the only place we intentionally want to insert the browser, yes, I believe so.  Of course we'll still need some traps so we don't break addons.
Here is my slightly educated guess of what addons may access:

canGoBack           getTabBrowser                     blockMedia
canGoForward        finder                            resumeMedia
goBack              fastFind                          userTypedValue
goForward           sessionHistory                    purgeSessionHistory
reload              contentTitle                      stopScroll
reloadWithFlags     characterSet                      startScroll
stop                mayEnableCharacterEncodingMenu    blockedPopups
loadURI             fullZoom                          
goHome              textZoom                          _userTypedValue
homePage            addProgressListener               mIconURL
gotoIndex           removeProgressListener            lastURI
currentURI          audioPlaybackStarted              mDestroyed
loadURIWithFlags    audioPlaybackStopped              _scrolling
documentURI         audioMuted                        _startX
preferences         mute                              _startY
imageDocument       unmute                
isRemoteBrowser     pauseMedia
messageManager      stopMedia

Not sure if they would access these:

docShell
docShellIsActive
webNavigation
webBrowserFind
outerWindowID
innerWindowID
webProgress
Here is the full list:

permanentKey                        sessionHistory                    userTypedValue
droppedLinkHandler                  markupDocumentViewer              destroy
_docShell                           contentViewerEdit                 _receiveMessage
loadURIWithFlags                    contentViewerFile                 receiveMessage
tabModalPromptBox                   contentDocument                   observe
autoscrollEnabled                   contentDocumentAsCPOW             purgeSessionHistory
canGoBack                           contentTitle                      stopScroll
canGoForward                        characterSet                      _createAutoScrollPopup
_wrapURIChangeCall                  mayEnableCharacterEncodingMenu    startScroll
goBack                              contentPrincipal                  handleEvent
goForward                           showWindowResizer                 swapDocShells
reload                              manifestURI                       getInPermitUnload
reloadWithFlags                     fullZoom                          permitUnload
stop                                textZoom                          print
loadURI                             isSyntheticDocument               _loadContext
goHome                              hasContentOpener                  _webNavigation
homePage                            mStrBundle                        _webBrowserFind
gotoIndex                           addProgressListener               _finder
currentURI                          removeProgressListener            _fastFind
_setCurrentURI                      attachFormFill                    _lastSearchString
documentURI                         detachFormFill                    _contentWindow
documentContentType                 findChildShell                    mPrefs
preferences                         onPageShow                        _mStrBundle
docShell                            onPageHide                        blockedPopups
loadContext                         updateBlockedPopups               _audioMuted
docShellIsActive                    retrieveListOfBlockedPopups       urlbarChangeTracker
setDocShellIsActiveAndForeground    unblockPopup                      _userTypedValue
makePrerenderedBrowserActive        pageReport                        mFormFillAttached
imageDocument                       audioPlaybackStarted              isShowingMessage
isRemoteBrowser                     audioPlaybackStopped              mIconURL
messageManager                      audioMuted                        lastURI
webNavigation                       mute                              mDestroyed
webBrowserFind                      unmute                            _AUTOSCROLL_SNAP
getTabBrowser                       pauseMedia                        _scrolling
finder                              stopMedia                         _startX
fastFind                            blockMedia                        _startY
outerWindowID                       resumeMedia                       _autoScrollPopup
innerWindowID                       securityUI                        _autoScrollNeedsCleanup
webProgress                         adjustPriority                    _alive
contentWindow                       setPriority
contentWindowAsCPOW                 didStartLoadSinceLastUserTyping
Posted patch Patch V13 (obsolete) — Splinter Review
This uses _createBrowser right off, like you suggested.  This moves a lot of browser related code back into addTab.  I like this because there is a lot more logical flow, and the lazy-browser isn't so mysterious now.

I also eliminated `createBrowserPropertyNamesList()` and I explicitly set some suggested values for `_browserPropertyNamesList`.
Attachment #8774664 - Attachment is obsolete: true
Attachment #8776828 - Attachment is obsolete: true
Attachment #8776978 - Flags: feedback?(dao+bmo)
Assignee: nobody → allassopraise
A little cleaner version.

I have been working with SessionStore.jsm and satellite consumers and I'm finding this new approach cleans things up a lot.  Many code changes previously needed are simpler and some changes aren't required now.
Attachment #8776978 - Attachment is obsolete: true
Attachment #8776978 - Flags: feedback?(dao+bmo)
Attachment #8777237 - Flags: feedback?(dao+bmo)
Attachment #8777237 - Attachment is patch: true
Comment on attachment 8777237 [details] [diff] [review]
Patch V14 - no proxy, actual browser

>+            // ADVISE: using for-of causes consumers to break down - why?
>+            //for (let name of names) {

What consumers? Break how?

Probably makes sense to rename _linkBrowserToTab now... _insertBrowser or something like that.

_browserPropertyNamesList should be renamed such that it's clear that these properties need the browser to be in the DOM.

_scrolling is private and shouldn't instantiate the browser. Not sure what _startX and _startY are.

Please get rid of isFullyInstantiated and just check browser.parentNode instead.

Not sure what the change to the TabBrowserCreated event is about. Looks like it doesn't belong in this bug.
Attachment #8777237 - Flags: feedback?(dao+bmo) → feedback+
(In reply to Dão Gottwald [:dao] from comment #72)
> Comment on attachment 8777237 [details] [diff] [review]
> Patch V14 - no proxy, actual browser
> 
> >+            // ADVISE: using for-of causes consumers to break down - why?
> >+            //for (let name of names) {
> 
> What consumers? Break how?

If I use the `for-of` construct, start firefox, and run `gBrowser.addTab().linkedBrowser.loadURI("about:robots");`, I get an error:

Exception: TypeError: gBrowser.addTab(...).linkedBrowser.loadURI is not a function

I think there were others as well.

If I use the `for` construct instead I don't have any problems whatsoever.  This makes absolutely no sense to me.

> Not sure what the change to the TabBrowserCreated event is about. Looks like
> it doesn't belong in this bug.

Yes, premature.  The `currentURI` trap is premature also.
> _browserPropertyNamesList should be renamed such that it's clear that these properties
> need the browser to be in the DOM.

How about _browserXBLPropertyNames?
Well, what exactly is this array? Does it contain all XBL properties and nothing else?
(In reply to Dão Gottwald [:dao] from comment #75)
> Well, what exactly is this array? Does it contain all XBL properties and
> nothing else?

Yes.
Then that seems fine, although I'd call it _browserBindingProperties.
> Please get rid of isFullyInstantiated and just check browser.parentNode instead.

That won't work, browser gets appended to the bottom of sub-tree in `_createBrowser`.  I'd have to use

`browser.parentNode.parentNode.parentNode.parentNode.parentNode`
You can also just check aTab.linkedPanel.
(In reply to Dão Gottwald [:dao] from comment #79)
> You can also just check aTab.linkedPanel.

Ooooh, I wanted to stay away from having to access the tab.  One of the reasons code changes in consumers have become simpler is because I can test a property on the browser.
(In reply to Allasso Travesser from comment #80)
> (In reply to Dão Gottwald [:dao] from comment #79)
> > You can also just check aTab.linkedPanel.
> 
> Ooooh, I wanted to stay away from having to access the tab.  One of the
> reasons code changes in consumers have become simpler is because I can test
> a property on the browser.

In some cases `tab` is already present, so maybe it would be okay.

We also could test for any browser property that is only present after binding, but would that be too hackish?
In the places where you're currently using isFullyInstantiated, you already had the tab in the first place. If this property is somehow useful in other contexts, let's worry about adding it when we're there.
Okay.(In reply to Dão Gottwald [:dao] from comment #82)
> In the places where you're currently using isFullyInstantiated, you already
> had the tab in the first place. If this property is somehow useful in other
> contexts, let's worry about adding it when we're there.

Sure, that makes sense.
Posted patch Patch V15 (obsolete) — Splinter Review
Attachment #8777237 - Attachment is obsolete: true
Attachment #8777300 - Flags: review?(dao+bmo)
Comment on attachment 8777300 [details] [diff] [review]
Patch V15

>+      <field name="_browserBindingProperties">
>+        [
>+          "canGoBack", "canGoForward", "goBack", "goForward",
>+          "reload", "reloadWithFlags", "stop", "loadURI", "loadURIWithFlags",
>+          "goHome", "homePage", "gotoIndex", "currentURI", "documentURI",
>+          "preferences", "imageDocument", "isRemoteBrowser", "messageManager",
>+          "getTabBrowser", "finder", "fastFind", "sessionHistory",
>+          "contentTitle", "characterSet", "fullZoom", "textZoom",
>+          "addProgressListener", "removeProgressListener",
>+          "audioPlaybackStarted", "audioPlaybackStopped",
>+          "audioMuted", "_audioMuted", "mute", "unmute",
>+          "pauseMedia", "stopMedia", "blockMedia", "resumeMedia",
>+          "userTypedValue", "purgeSessionHistory", "stopScroll", "startScroll",
>+          "blockedPopups", "_userTypedValue", "mIconURL", "lastURI"
>+        ]

_audioMuted and _userTypedValue are also private. (And so is mIconURL, technically, but pretty sure it's used externally...)

>+      <method name="_createLazyBrowser">
>         <parameter name="aTab"/>
>-        <parameter name="aURI"/>
>-        <parameter name="aParams"/>
>+        <body>
>+          <![CDATA[
>+            let browser = aTab.linkedBrowser;
>+
>+            let names = this._browserBindingProperties;
>+
>+            for (let i = 0; i < names.length; i++) {
>+              let name = names[i];
>+              let getter;
>+              let setter;
>+              switch (name) {
>+                default:
>+                  getter = () => {
>+                    gBrowser.instantiateLazyBrowser(aTab);
>+                    return browser[name];
>+                  }
>+                  setter = (value) => {
>+                    gBrowser.instantiateLazyBrowser(aTab);

Please use 'this' instead of gBrowser.

>+      <method name="instantiateLazyBrowser">

It looks like this is really just a thin wrapper around _insertBrowser. Can you fold this code into _insertBrowser and call that instead of instantiateLazyBrowser?

>+      <method name="_insertBrowser">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+            "use strict";
>+
>+            let { uri, remote, usingPreloadedContent } = aTab._browserParams;
>+            delete(aTab._browserParams);
>+
>+            let uriIsAboutBlank = !uri || uri == "about:blank";
>+
>+            let browser = aTab.linkedBrowser;
>
>             let notificationbox = this.getNotificationBox(browser);
>             let uniqueId = this._generateUniquePanelID();
>             notificationbox.id = uniqueId;
>             aTab.linkedPanel = uniqueId;
>-            aTab.linkedBrowser = browser;
>-            aTab.hasBrowser = true;
>-            this._tabForBrowser.set(browser, aTab);
>
>             // Inject the <browser> into the DOM if necessary.
>             if (!notificationbox.parentNode) {

Inserting the browser should always be necessary, right? Since that's the point of this method...
Attachment #8777300 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #85)
> Please use 'this' instead of gBrowser.

Hmm, I thought `this` != gBrowser in that context, but I see now that it is.

> >+      <method name="instantiateLazyBrowser">
> 
> It looks like this is really just a thin wrapper around _insertBrowser. Can
> you fold this code into _insertBrowser and call that instead of
> instantiateLazyBrowser?

So add a parameter to _insertBrowser which would flag whether or not to delete the substitute binding properties on the lazy browser?  (They don't exist if _insertBrowser is called explicitly from addTab.)
(In reply to Allasso Travesser from comment #86)
> (In reply to Dão Gottwald [:dao] from comment #85)
> > Please use 'this' instead of gBrowser.
> 
> Hmm, I thought `this` != gBrowser in that context, but I see now that it is.
> 
> > >+      <method name="instantiateLazyBrowser">
> > 
> > It looks like this is really just a thin wrapper around _insertBrowser. Can
> > you fold this code into _insertBrowser and call that instead of
> > instantiateLazyBrowser?
> 
> So add a parameter to _insertBrowser which would flag whether or not to
> delete the substitute binding properties on the lazy browser?  (They don't
> exist if _insertBrowser is called explicitly from addTab.)

One kink I can think of in that, is that in upcoming code, we'll need a public method anyway to be able to instantiate the browser, particularly in SessionStore.  `instantiateLazyBrowser` covers both needs.
(In reply to Allasso Travesser from comment #86)
> > It looks like this is really just a thin wrapper around _insertBrowser. Can
> > you fold this code into _insertBrowser and call that instead of
> > instantiateLazyBrowser?
> 
> So add a parameter to _insertBrowser which would flag whether or not to
> delete the substitute binding properties on the lazy browser?  (They don't
> exist if _insertBrowser is called explicitly from addTab.)

Does it matter? Can't you just delete them regardless?
(In reply to Dão Gottwald [:dao] from comment #88)
> (In reply to Allasso Travesser from comment #86)
> > > It looks like this is really just a thin wrapper around _insertBrowser. Can
> > > you fold this code into _insertBrowser and call that instead of
> > > instantiateLazyBrowser?
> > 
> > So add a parameter to _insertBrowser which would flag whether or not to
> > delete the substitute binding properties on the lazy browser?  (They don't
> > exist if _insertBrowser is called explicitly from addTab.)
> 
> Does it matter? Can't you just delete them regardless?

I guess, just seems like a waste of resources since those properties won't be on the non-lazy browser.
delete should be extremely cheap, but I suppose you could still wrap it in a check like if (this._browserBindingProperties[0] in browser) {...}.
Posted patch Patch V16 (obsolete) — Splinter Review
Attachment #8777300 - Attachment is obsolete: true
Attachment #8777342 - Flags: review?(dao+bmo)
Posted patch Patch V17 (obsolete) — Splinter Review
A little more clean-up.
Attachment #8777342 - Attachment is obsolete: true
Attachment #8777342 - Flags: review?(dao+bmo)
Attachment #8777347 - Flags: review?(dao+bmo)
Comment on attachment 8777347 [details] [diff] [review]
Patch V17

>+      <field name="_browserBindingProperties">

please add some documentation here

>+                  getter = () => {
>+                    this._insertBrowser(aTab);
>+                    return browser[name];
>+                  }

nit: missing semicolon

>+                  setter = (value) => {
>+                    this._insertBrowser(aTab);
>+                    return browser[name] = value;
>+                  }

ditto
Attachment #8777347 - Flags: review?(dao+bmo) → review+
webProgress and permitUnload is missing from _browserBindingProperties:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=242fe74ca751
Posted patch Patch V18 (obsolete) — Splinter Review
Updated per comment 93, comment 94.  After updates additional errors were found:

Added `adjustPriority` to `_browserBindingProperties` as well.

When `updateBrowserRemoteness` or `_swapBrowserDocShells` is called on a lazy tab, error is thrown because `filter = this._tabFilters.get(tab)` is undefined.  Added code in those methods to ensure lazy browsers get instantiated.

With these changes all previously failed tests pass.
Attachment #8777347 - Attachment is obsolete: true
Attachment #8777822 - Flags: review?(dao+bmo)
Posted patch Patch V19 (obsolete) — Splinter Review
Removed the redundant test for tab.linkedPanel in `getRelatedElement`.
Attachment #8777846 - Flags: review?(dao+bmo)
Attachment #8777822 - Attachment is obsolete: true
Attachment #8777822 - Flags: review?(dao+bmo)
Patch V19 ran clean:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4035ef2e7f79

I'll fix the semi-colon nit and upload a new patch.
Posted patch Patch V20 (obsolete) — Splinter Review
Removed unnecessary semi-colon in _createLazyBrowser.
Attachment #8777846 - Attachment is obsolete: true
Attachment #8777846 - Flags: review?(dao+bmo)
Attachment #8778304 - Flags: review?(dao+bmo)
Do you think we should change the name of `TabBrowserCreated` event?
Yeah, probably TabBrowserInserted.
Comment on attachment 8778304 [details] [diff] [review]
Patch V20

>-            // Inject the <browser> into the DOM if necessary.
>+
>             if (!notificationbox.parentNode) {

You removed the comment but left the check. I don't think the check makes sense.
Okay, new patch coming right up.
Posted patch Patch V21 (obsolete) — Splinter Review
Here it is
Attachment #8778304 - Attachment is obsolete: true
Attachment #8778304 - Flags: review?(dao+bmo)
Comment on attachment 8778421 [details] [diff] [review]
Patch V21

Sorry, this has a revision error.  I'll try again
Attachment #8778421 - Attachment is obsolete: true
Posted patch Patch V22 (obsolete) — Splinter Review
Try this
Posted patch Patch V23 (obsolete) — Splinter Review
Attachment #8778427 - Attachment is obsolete: true
Posted patch Patch V24 (obsolete) — Splinter Review
Sorry for the multiple uploads, I believe this should take care of it.
Attachment #8778482 - Attachment is obsolete: true
Attachment #8778696 - Flags: review?(dao+bmo)
Posted patch Patch V25 (obsolete) — Splinter Review
Removing the test for `notificationbox.parentNode` in `_insertBrowser` was causing the following tests to fail:

browser/base/content/test/newtab/browser_newtab_bug1145428.js
browser/base/content/test/newtab/browser_newtab_bug1178586.js
browser/base/content/test/newtab/browser_newtab_bug722273.js
browser/base/content/test/newtab/browser_newtab_bug725996.js

This patch re-instates the test.
Attachment #8778696 - Attachment is obsolete: true
Attachment #8778696 - Flags: review?(dao+bmo)
Attachment #8782630 - Flags: review?(dao+bmo)
(In reply to Allasso Travesser from comment #109)
> Created attachment 8782630 [details] [diff] [review]
> Patch V25
> 
> Removing the test for `notificationbox.parentNode` in `_insertBrowser` was
> causing the following tests to fail:
> 
> browser/base/content/test/newtab/browser_newtab_bug1145428.js
> browser/base/content/test/newtab/browser_newtab_bug1178586.js
> browser/base/content/test/newtab/browser_newtab_bug722273.js
> browser/base/content/test/newtab/browser_newtab_bug725996.js
> 
> This patch re-instates the test.

I observe that removing the test on a default build (no patch applied) causes the same failures.
Comment on attachment 8782630 [details] [diff] [review]
Patch V25

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1574,16 +1574,18 @@
>
>             let wasActive = document.activeElement == aBrowser;
>
>             // Unmap the old outerWindowID.
>             this._outerWindowIDBrowserMap.delete(aBrowser.outerWindowID);
>
>             // Unhook our progress listener.
>             let tab = this.getTabForBrowser(aBrowser);
>+            // aBrowser may be lazy, if so force `_insertBrowser`.
>+            this._insertBrowser(tab);

How about:

            // aBrowser needs to be inserted now if it hasn't been already.

>       <method name="_swapBrowserDocShells">
>         <parameter name="aOurTab"/>
>         <parameter name="aOtherBrowser"/>
>         <body>
>           <![CDATA[
>+            // aOurTab's browser may be lazy, if so force `_insertBrowser`
>+            this._insertBrowser(aOurTab);

ditto
Attachment #8782630 - Flags: review?(dao+bmo) → review+
Posted patch Patch V26 (obsolete) — Splinter Review
Changed comments
Attachment #8782630 - Attachment is obsolete: true
Attachment #8782879 - Flags: review?(dao+bmo)
Comment on attachment 8782879 [details] [diff] [review]
Patch V26

Okay, I'll try to land this *fingers crossed*
Attachment #8782879 - Flags: review?(dao+bmo) → review+
Summary: Lazy-browser Tabs - add proxy for linkedBrowser → Insert tabs' linkedBrowser lazily into the document
So the patch applies on mozilla-central, but apparently there are conflicting changes incoming from mozilla-inbound, where the patch fails to apply in tabbrowser.xml.
(In reply to Dão Gottwald [:dao] from comment #115)
> So the patch applies on mozilla-central, but apparently there are
> conflicting changes incoming from mozilla-inbound, where the patch fails to
> apply in tabbrowser.xml.

https://hg.mozilla.org/integration/mozilla-inbound/rev/02ededf61cbe
(In reply to Dão Gottwald [:dao] from comment #116)
> (In reply to Dão Gottwald [:dao] from comment #115)
> > So the patch applies on mozilla-central, but apparently there are
> > conflicting changes incoming from mozilla-inbound, where the patch fails to
> > apply in tabbrowser.xml.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/02ededf61cbe

Do I need to wait until it is on mozilla-central and update my local repos to generate another patch, or is there another way to do this?
I think you can pull from inbound in central, see hg pull --help
(In reply to Dão Gottwald [:dao] from comment #118)
> I think you can pull from inbound in central, see hg pull --help

I don't want to break anything:

hg pull https://hg.mozilla.org/integration/mozilla-inbound/

Does that look right?
Never mind, I went for it.
Posted patch Patch V27 (obsolete) — Splinter Review
Try this one...
Attachment #8782879 - Attachment is obsolete: true
Attachment #8782915 - Flags: review?(dao+bmo)
Attachment #8782915 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/865501b808e3
Insert tabs' linkedBrowser lazily into the document. r=dao
https://hg.mozilla.org/mozilla-central/rev/865501b808e3
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Backed this out for frequent timeouts in browser_async_remove_tab.js with e10s on Linux debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a646dca439b

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=865501b808e3f0e634220b15f18fcd2280b1dd2f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=34260968&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(allassopraise)
Resolution: FIXED → ---
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #124)
> Backed this out for frequent timeouts in browser_async_remove_tab.js with
> e10s on Linux debug:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7a646dca439b
> 
> Push with failure:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=865501b808e3f0e634220b15f18fcd2280b1dd2f
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=34260968&repo=mozilla-
> inbound

This seems intermittent for me.  It passes more often than it fails.

Could this be a bug in the test?
Flags: needinfo?(dao+bmo)
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(allassopraise)
(In reply to Allasso Travesser from comment #125)
> This seems intermittent for me.  It passes more often than it fails.
> 
> Could this be a bug in the test?

Maybe this one?  bug 1254852
(In reply to Allasso Travesser from comment #126)
> (In reply to Allasso Travesser from comment #125)
> > This seems intermittent for me.  It passes more often than it fails.
> > 
> > Could this be a bug in the test?
> 
> Maybe this one?  bug 1254852

No, this is a new problem, and it definitely started just after this landed. I kept running into it when I was looking for failures from one of my patches.

Based on the test output, it looks like this might be causing problems for the cycle collector. There are about 60 about:blank windows from much earlier tests collected during that test.
Flags: needinfo?(aryx.bugmail)
(In reply to Kris Maglione [:kmag] from comment #127)
> (In reply to Allasso Travesser from comment #126)
> > (In reply to Allasso Travesser from comment #125)
> > > This seems intermittent for me.  It passes more often than it fails.
> > > 
> > > Could this be a bug in the test?
> > 
> > Maybe this one?  bug 1254852
> 
> No, this is a new problem, and it definitely started just after this landed.
> I kept running into it when I was looking for failures from one of my
> patches.
> 
> Based on the test output, it looks like this might be causing problems for
> the cycle collector. There are about 60 about:blank windows from much
> earlier tests collected during that test.

Any suggestions on how to troubleshoot?
> Any suggestions on how to troubleshoot?

I observe that when it does fail, it times out after all of the tests are completed successfully.
It doesn't time out, it just takes longer than 45 seconds to complete.

The only possible suggestion I have is to make sure you remove the getters from the browsers of tabs that are removed before the browsers are attached.
(In reply to Kris Maglione [:kmag] from comment #130)
> It doesn't time out, it just takes longer than 45 seconds to complete.
> 
> The only possible suggestion I have is to make sure you remove the getters
> from the browsers of tabs that are removed before the browsers are attached.

Actually, in this phase of bug 906076, I think it best that we just ensure the browser is inserted before running any `removeTab` code.  We are not actually achieving any optimization at this point.  Currently this happens automatically on a lazy tab anyway through the getters, but in some cases this may occur too late.

Once we are at the phase of using optimization, we will need to re-think `removeTab` altogether to handle lazy tabs, since it uses code that is expecting that linkedBrowser is inserted into the document and fully instantiated.

I will write a patch that forces browser insertion before removing tabs, and we can see if that fixes browser_async_remove_tab.js test failures.
Posted patch Patch V28 (obsolete) — Splinter Review
This is an attempt to fix browser_async_remove_tab.js failure.

Kris, can you see if this takes care of the issue in your situation?  I am unable to get a failure, but I was not able to before the fix either.
Attachment #8782915 - Attachment is obsolete: true
Attachment #8783360 - Flags: feedback?(kmaglione+bmo)
Attachment #8783360 - Flags: feedback?(dao+bmo)
Depends on: 1297005
Attachment #8783360 - Attachment is obsolete: true
Attachment #8783360 - Flags: feedback?(kmaglione+bmo)
Attachment #8783360 - Flags: feedback?(dao+bmo)
Merge of backout: https://hg.mozilla.org/mozilla-central/rev/7a646dca439b
Target Milestone: Firefox 51 → ---
Posted patch Patch V29 (obsolete) — Splinter Review
Updated patch includes fix for bug 1297005, and (hopefully) fix for test failure in browser_async_remove_tab.js (comment 124).
Comment on attachment 8783682 [details] [diff] [review]
Patch V29

Feedback please.  Kris, are you able to see if you still get the failure in browser_async_remove_tab.js in your situation?

Re:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93936e0ca3f&selectedJob=26157389

I don't know if the failures against browser_relatedTabs.js and browser_addonPerformanceAlerts_2.js apply here, but I cannot duplicate them.  They both have bugs filed against them for the same failure descriptions.
Attachment #8783682 - Flags: feedback?(kmaglione+bmo)
Attachment #8783682 - Flags: feedback?(dao+bmo)
Comment on attachment 8783682 [details] [diff] [review]
Patch V29

>@@ -2268,16 +2340,22 @@
>         <body>
>           <![CDATA[
>             if (aParams) {
>               var animate = aParams.animate;
>               var byMouse = aParams.byMouse;
>               var skipPermitUnload = aParams.skipPermitUnload;
>             }
>
>+            // Ensure aTab's browser is inserted into the document.  In later
>+            // stages of bug 906076 where we actually utilize lazy-browser for
>+            // optimization,  `removeTab` API will have to be re-written to
>+            // accommodate lazy-browser tabs.
>+            this._insertBrowser(aTab);

Please move this to _beginRemoveTab. And you can get rid of the comment past the first sentence. Comments that try to predict the future often end up being wrong.
Flags: needinfo?(dao+bmo)
Attachment #8783682 - Flags: feedback?(dao+bmo) → feedback+
(In reply to Dão Gottwald [:dao] from comment #139)
> Comment on attachment 8783682 [details] [diff] [review]
> Patch V29
> 
> >@@ -2268,16 +2340,22 @@
> >         <body>
> >           <![CDATA[
> >             if (aParams) {
> >               var animate = aParams.animate;
> >               var byMouse = aParams.byMouse;
> >               var skipPermitUnload = aParams.skipPermitUnload;
> >             }
> >
> >+            // Ensure aTab's browser is inserted into the document.  In later
> >+            // stages of bug 906076 where we actually utilize lazy-browser for
> >+            // optimization,  `removeTab` API will have to be re-written to
> >+            // accommodate lazy-browser tabs.
> >+            this._insertBrowser(aTab);
> 
> Please move this to _beginRemoveTab.

It is possible that _endRemoveTab will be called immediately after its current position:

            this._insertBrowser(aTab);

            // Handle requests for synchronously removing an already
            // asynchronously closing tab.
            if (!animate &&
                aTab.closing) {
              this._endRemoveTab(aTab);
              return;
            }

_endRemoveTab does operations on the browser which may be affected if the browser is not inserted.
Flags: needinfo?(dao+bmo)
_beginRemoveTab has already been called in that case (hence aTab.closing).
Flags: needinfo?(dao+bmo)
Okay.
Comment on attachment 8783682 [details] [diff] [review]
Patch V29

Review of attachment 8783682 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't have time to test this right now. If you're having trouble reproducing it yourself, all I can give are some suggestions:

1) Make sure you're running the entire test directory.
2) If you're having trouble reproducing locally, try running under rr. Sometimes that slows things down enough to trigger bugs that you usually only see on infra.
3) If that fails, try using the One Click Loaner[1] feature on Taskcluster, which should let you experiment with changes without having to wait for a whole new build.

[1]: https://ahal.ca/blog/2016/taskcluster-interactive-loaner/
Attachment #8783682 - Flags: feedback?(kmaglione+bmo)
Here are some tips for debugging intermittents from the DevTools team[1].  DevTools also uses browser mochitests, so most of the tips should still apply to general browser tests.

[1]: https://wiki.mozilla.org/DevTools/Intermittents
Allasso, what's the current status here? I'm not exactly sure what you were asking Kris about... You've used the try server to test that your latest patch fixes the failures, right? What's left to do? How can I help? Or are you just busy with other things?
Flags: needinfo?(allassopraise)
(In reply to Dão Gottwald [:dao] from comment #145)
> Allasso, what's the current status here? I'm not exactly sure what you were
> asking Kris about... You've used the try server to test that your latest
> patch fixes the failures, right? What's left to do? How can I help?

I have been able to consistently reproduce the issue on Linux debug build on a Linux machine.  Running the entire suite is not necessary; I can reproduce by only running browser_async_remove_tab.js.  All tests in this file complete successfully, however for some reason the script just hangs at the end after all tests are completed.  Forcing browser insertion on about:blank tabs prior to calling `promiseBrowserLoaded` "fixes" the issue.

I have spent much time on this, removing certain tests, injecting code, etc, in search for a repeatable pattern, but without success.  Admittedly I am up against the current limits of my experience and familiarity of the technology, and I have not been able to determine a diagnosis or solution.  Naively guessing it seems to me there is a race issue going on, as this problem only seems to appear on debug builds.

> Or are you just busy with other things?

My experience and skill level is limited, and as this project is a "spare time" endeavor for me, it is often difficult for me to find time to invest on issues which require a steep learning curve.
Flags: needinfo?(allassopraise)
(In reply to Dão Gottwald [:dao] from comment #145)
> You've used the try server to test that your latest
> patch fixes the failures, right?

Just to be clear, using the try server revealed that my latest patch did not fix the failures.
Posted patch Patch V30 wip record (obsolete) — Splinter Review
WIP patch uploaded for record-keeping purposes
(In reply to Allasso Travesser from comment #148)
> Created attachment 8805734 [details] [diff] [review]
> Patch V30 wip record
> 
> WIP patch uploaded for record-keeping purposes

What changed here compared to the previous revision?

FWIW, I intent to look into this bug soon again to help unblock it.
Flags: needinfo?(allassopraise)
(In reply to Dão Gottwald [:dao] from comment #149)
> What changed here compared to the previous revision?

Nothing basically, just brought the patch up to date with latest mozilla-central.

> FWIW, I intent to look into this bug soon again to help unblock it.

Very much appreciated.  I observe that if I increase the timeout to 55 seconds, the problem goes away.  As I stated before, all of the addTask tests pass okay, it is after that where it times out.
Flags: needinfo?(allassopraise)
(In reply to Allasso Travesser from comment #150)
> all of the addTask tests pass okay

Should be `add_task`.
I also observe that there are no leaks reported from the final leak report.  Would this indicate that it is not a problem with gc? (see comment 127)
Flags: needinfo?(dao+bmo)
Allasso Travesser is now Kevin Jones.
Posted patch patch v30, rebased (obsolete) — Splinter Review
Attachment #8783682 - Attachment is obsolete: true
Attachment #8805734 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #154)
> Created attachment 8807126 [details] [diff] [review]
> patch v30, rebased

This appears to be an update/cleanup.  Is that all it is, or do these changes affect the browser_async_remove_tab.js issue?
(In reply to Kevin Jones from comment #155)
> (In reply to Dão Gottwald [:dao] from comment #154)
> > Created attachment 8807126 [details] [diff] [review]
> > patch v30, rebased
> 
> This appears to be an update/cleanup.  Is that all it is, or do these
> changes affect the browser_async_remove_tab.js issue?

I had to manually rebase and did some cleanup along the way. There should be no functional changes.

Here's a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c754272a80f52486c009fd5de3e67255a14b0e

I don't see a browser_async_remove_tab.js failure, but instead browser_webpagePerformanceAlerts.js, browser_relatedTabs.js and browser_tab_dragdrop2.js time out.

Also, there's this one:
TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug521216.js | uncaught exception - TypeError: invalid 'in' operand browser at _insertBrowser@chrome://browser/content/tabbrowser.xml:2075:1

If I remove the (this._browserBindingProperties[0] in browser) check, there's no problem with browser_bug521216.js:

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

browser_webpagePerformanceAlerts.js and browser_tab_dragdrop2.js are still an issue. browser_relatedTabs.js didn't time out here but that's probably just intermittent.
Flags: needinfo?(dao+bmo)
Depends on: 1315222
Posted patch rebased (obsolete) — Splinter Review
> Also, there's this one:
> TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_bug521216.js | uncaught exception
> - TypeError: invalid 'in' operand browser at
> _insertBrowser@chrome://browser/content/tabbrowser.xml:2075:1
> 
> If I remove the (this._browserBindingProperties[0] in browser) check,
> there's no problem with browser_bug521216.js:

I still haven't removed that check in this patch. The underlying problem appears to be that aTab.linkedBrowser can be null in _insertBrowser, which shouldn't really be possible, so I think there's a bug somewhere in this patch allowing a tab not to have a browser.
Attachment #8807126 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #157)

> I still haven't removed that check in this patch. The underlying problem
> appears to be that aTab.linkedBrowser can be null in _insertBrowser, which
> shouldn't really be possible, so I think there's a bug somewhere in this
> patch allowing a tab not to have a browser.

Okay, sounds like some progress.  I hope to make some time to check into that in the next day or two if you don't nail it before then.  Thank you.
(In reply to Dão Gottwald [:dao] from comment #157)
> Created attachment 8808138 [details] [diff] [review]
> rebased
> 
> > Also, there's this one:
> > TEST-UNEXPECTED-FAIL |
> > browser/base/content/test/general/browser_bug521216.js | uncaught exception
> > - TypeError: invalid 'in' operand browser at
> > _insertBrowser@chrome://browser/content/tabbrowser.xml:2075:1
> > 
> > If I remove the (this._browserBindingProperties[0] in browser) check,
> > there's no problem with browser_bug521216.js:
> 
> I still haven't removed that check in this patch. The underlying problem
> appears to be that aTab.linkedBrowser can be null in _insertBrowser, which
> shouldn't really be possible, so I think there's a bug somewhere in this
> patch allowing a tab not to have a browser.

I haven't been able to get a failure on browser_bug521216.js locally on Linux debug.  I'm thinking though, if `browser` were null and you remove the check, shouldn't you get a failure anyway when you try to `delete browser[name]`?
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #159)
> (In reply to Dão Gottwald [:dao] from comment #157)
> > Created attachment 8808138 [details] [diff] [review]
> > rebased
> > 
> > > Also, there's this one:
> > > TEST-UNEXPECTED-FAIL |
> > > browser/base/content/test/general/browser_bug521216.js | uncaught exception
> > > - TypeError: invalid 'in' operand browser at
> > > _insertBrowser@chrome://browser/content/tabbrowser.xml:2075:1
> > > 
> > > If I remove the (this._browserBindingProperties[0] in browser) check,
> > > there's no problem with browser_bug521216.js:
> > 
> > I still haven't removed that check in this patch. The underlying problem
> > appears to be that aTab.linkedBrowser can be null in _insertBrowser, which
> > shouldn't really be possible, so I think there's a bug somewhere in this
> > patch allowing a tab not to have a browser.
> 
> I haven't been able to get a failure on browser_bug521216.js locally on
> Linux debug.  I'm thinking though, if `browser` were null and you remove the
> check, shouldn't you get a failure anyway when you try to `delete
> browser[name]`?

It appears to be intermittent. I got that failure in a later try run.
Flags: needinfo?(dao+bmo)
HELP!

I am sorry I have not been able to give this issue (test failures) any attention for quite some time.  I have spent quite a bit of time working on this issue and feel I am up against my current limits of experience to be able to debug this at the present time.  Everything I have seen tells me this is not simply a bug in the patch per se [1], but something along the lines of a race condition or a garbage collection issue as has been suggested (comment 127).  If I had the time, I would chip away at it, but right now navigating the learning curve in areas of which I am quite unfamiliar is just not possible for me.  All of my Firefox attention has been going to spending a lot of time trying to keep my addons afloat through the WebExtensions migration.  All this notwithstanding keeping my "day job".

This bug promises much reward in terms of performance optimization, and many many hours of work has gone into it so far, yet it has been hung up on this one snag in the path for quite some time.  It is nearing the point where we could actually begin reaping some of the benefits of promised optimization, and I would like it not to fall by the wayside.  If there is anyone out there with higher level of skill who would be willing to invest some time and look into the reason for these test failures and how to correct them, it would be very much appreciated.  If I could just get past this snag I would then feel free to resume the course and continue spending time on this to move it on to reality.

I have been able to duplicate the browser_async_remove_tab.js consistently on Linux debug locally and demonstrate that this patch is indeed the cause of that particular test failure.  Some of the other failures mentioned here may just be oranges being oranges.

[1] Regarding comment 157, if `browser` were null and you remove the check, you should you get a failure anyway when you try to `delete browser[name]`
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(dao+bmo)
needinfo'ing David regarding comment 161
Flags: needinfo?(dteller)
Kevin and Dão,

I tried to help but I failed to properly reproduce the failing tests.

This is what I did:


#!/bin/bash

mkdir patches
wget -O patches/8808138.diff https://bugzilla.mozilla.org/attachment.cgi?id=8808138
hg clone -U http://hg.mozilla.org/mozilla-central
cd mozilla-central
hg up add9dada238ed99b4f93c027b535423f067d3781
patch <../patches/8808138.diff -p1
echo  >.mozconfig 'ac_add_options --enable-debug'
echo >>.mozconfig 'mk_add_options CC=gcc-5'
./mach configure 
./mach clobber
./mach build
./mach mochitest toolkit/components/perfmonitoring/tests/browser/browser_webpagePerformanceAlerts.js
./mach mochitest browser/base/content/test/general/browser_bug521216.js


Unfortunately, the test pass and do not fail:

[...]
Browser Chrome Test Summary
        Passed: 27
        Failed: 0
        Todo: 0
        Mode: e10s
[...]
Browser Chrome Test Summary
        Passed: 2
        Failed: 0
        Todo: 0
        Mode: e10s
[...]

What exactly should I do to reproduce the failing tests?
(In reply to Xuân Baldauf from comment #163)
> Kevin and Dão,
> 
> I tried to help but I failed to properly reproduce the failing tests.
> 
> This is what I did:
> 
> 
> #!/bin/bash
> 
> mkdir patches
> wget -O patches/8808138.diff
> https://bugzilla.mozilla.org/attachment.cgi?id=8808138
> hg clone -U http://hg.mozilla.org/mozilla-central
> cd mozilla-central
> hg up add9dada238ed99b4f93c027b535423f067d3781
> patch <../patches/8808138.diff -p1
> echo  >.mozconfig 'ac_add_options --enable-debug'
> echo >>.mozconfig 'mk_add_options CC=gcc-5'
> ./mach configure 
> ./mach clobber
> ./mach build
> ./mach mochitest
> toolkit/components/perfmonitoring/tests/browser/
> browser_webpagePerformanceAlerts.js
> ./mach mochitest browser/base/content/test/general/browser_bug521216.js
> 
> 
> Unfortunately, the test pass and do not fail:
> 
> [...]
> Browser Chrome Test Summary
>         Passed: 27
>         Failed: 0
>         Todo: 0
>         Mode: e10s
> [...]
> Browser Chrome Test Summary
>         Passed: 2
>         Failed: 0
>         Todo: 0
>         Mode: e10s
> [...]
> 
> What exactly should I do to reproduce the failing tests?

Hello Xuan,

Thanks for your help!  Actually, the only test I could consistently reproduce the failures on was:

browser_async_remove_tab.js (don't have the full path handy right now, you could `find` for it).

It appears that that is the one test you didn't run.

I was testing on a Linux machine, using a debug build.

Also you might take a peek at the code to make sure the patches were applied properly.
Hello Kevin,

./mach mochitest browser/components/sessionstore/test/browser_async_remove_tab.js

also succeeds:

[...]
Browser Chrome Test Summary
        Passed: 24
        Failed: 0
        Todo: 0
        Mode: e10s
[...]

My build is also a debug build ('ac_add_options --enable-debug') on x86_64 Linux.

The patch "8808138.diff" applied without errors. However, it is only one patch. Do I have to apply multiple patches in sequence?
Hello Xuan,

I think that should be the only patch you need to apply, though it is dated now so I don't know if there might be a conflict with latest mozilla-central.  I suppose you should have gotten an error though.

Can you try pushing to the try server and seeing if you still get failures?
(In reply to Kevin Jones from comment #166)
> Hello Xuan,
> 

Hello Kevin.

> I think that should be the only patch you need to apply, though it is dated
> now so I don't know if there might be a conflict with latest
> mozilla-central.

I apply the patch on revision add9dada238ed99b4f93c027b535423f067d3781, so possible conflicts with latest mozilla-central are not an issue.

> I suppose you should have gotten an error though.
> 
> Can you try pushing to the try server and seeing if you still get failures?

Unfortunately, I cannot, as I currently do not have access to the Try server.
Hello Kevin,

Given that it seems to take some time before I get access to the Try server, can _you_ push to the Try server or provide a script which reproduces the problem on a vanilla Linux machine?

This would be my script:

#!/bin/bash
mkdir patches
wget -O patches/8808138.diff https://bugzilla.mozilla.org/attachment.cgi?id=8808138
hg clone -U http://hg.mozilla.org/mozilla-central
cd mozilla-central
hg up add9dada238ed99b4f93c027b535423f067d3781
patch <../patches/8808138.diff -p1
echo  >.mozconfig 'ac_add_options --enable-debug'
echo >>.mozconfig 'mk_add_options CC=gcc-5'
./mach configure 
./mach clobber
./mach build
./mach mochitest toolkit/components/perfmonitoring/tests/browser/browser_webpagePerformanceAlerts.js
./mach mochitest browser/base/content/test/general/browser_bug521216.js
./mach mochitest browser/components/sessionstore/test/browser_async_remove_tab.js


Maybe you can run it, see that it does not reproduce the problem and then change it such that it reproduces the problem. After that, I (and others) can take over this and try to deal with the problem.
OK, I looked into this a bit, and the problem seems to be that browser_async_remove_tab calls BrowserTestUtils.browserLoaded to wait for lazily restored tabs to load. browserLoaded does not touch any of the properties that cause the browser to be inserted, so the test stalls until SessionSaverInternal.runDelayed runs and touches a property which causes the browser to be inserted. Adding "ownerDocument" to the list of binding properties fixes the immediate issue.

This means, incidentally, that the first time the session saver runs, all lazily restored tabs will have their browsers immediately inserted, and removes most of the usefulness of this patch...
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #169)
> OK, I looked into this a bit, and the problem seems to be that
> browser_async_remove_tab calls BrowserTestUtils.browserLoaded to wait for
> lazily restored tabs to load. browserLoaded does not touch any of the
> properties that cause the browser to be inserted, so the test stalls until
> SessionSaverInternal.runDelayed runs and touches a property which causes the
> browser to be inserted. Adding "ownerDocument" to the list of binding
> properties fixes the immediate issue.

Thanks a bunch, Kris, good work.  Initially the patch created getters for all properties in the binding, but it was decided that only certain properties were necessary.  It looks like we don't have them all covered.  Maybe we should go back to creating getters for all properties to cover any and all cases, present and future.

> This means, incidentally, that the first time the session saver runs, all
> lazily restored tabs will have their browsers immediately inserted, and
> removes most of the usefulness of this patch...

Actually, there are several ways the tabs get immediately inserted.  Session restore will do that also on startup.  We will not see any optimization until finding and examining all the consumers and rewriting code so the browsers don't get inserted until there is a demand.  Once this lands, that will be the next step.
(In reply to Xuân Baldauf from comment #168)
> Hello Kevin,
> 
> Given that it seems to take some time before I get access to the Try server,
> can _you_ push to the Try server or provide a script which reproduces the
> problem on a vanilla Linux machine?
> 
> This would be my script:
> 
> #!/bin/bash
> mkdir patches
> wget -O patches/8808138.diff
> https://bugzilla.mozilla.org/attachment.cgi?id=8808138
> hg clone -U http://hg.mozilla.org/mozilla-central
> cd mozilla-central
> hg up add9dada238ed99b4f93c027b535423f067d3781
> patch <../patches/8808138.diff -p1
> echo  >.mozconfig 'ac_add_options --enable-debug'
> echo >>.mozconfig 'mk_add_options CC=gcc-5'
> ./mach configure 
> ./mach clobber
> ./mach build
> ./mach mochitest
> toolkit/components/perfmonitoring/tests/browser/
> browser_webpagePerformanceAlerts.js
> ./mach mochitest browser/base/content/test/general/browser_bug521216.js
> ./mach mochitest
> browser/components/sessionstore/test/browser_async_remove_tab.js
> 
> 
> Maybe you can run it, see that it does not reproduce the problem and then
> change it such that it reproduces the problem. After that, I (and others)
> can take over this and try to deal with the problem.

Hello, Xuan, it looks like Kris found the problem, so this isn't necessary now.  Thanks very much for your willingness to help.
(In reply to Kevin Jones from comment #170)
> (In reply to Kris Maglione [:kmag] from comment #169)
> > OK, I looked into this a bit, and the problem seems to be that
> > browser_async_remove_tab calls BrowserTestUtils.browserLoaded to wait for
> > lazily restored tabs to load. browserLoaded does not touch any of the
> > properties that cause the browser to be inserted, so the test stalls until
> > SessionSaverInternal.runDelayed runs and touches a property which causes the
> > browser to be inserted. Adding "ownerDocument" to the list of binding
> > properties fixes the immediate issue.
> 
> Thanks a bunch, Kris, good work.  Initially the patch created getters for
> all properties in the binding, but it was decided that only certain
> properties were necessary.  It looks like we don't have them all covered. 
> Maybe we should go back to creating getters for all properties to cover any
> and all cases, present and future.

Dao, what do you think about this?  Should I just add `ownerDocument` to the list, or should we include the entire list of properties, like I did in attachment 8776828 [details] [diff] [review]?
Flags: needinfo?(dteller)
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
I don't think accessing ownerDocument / ownerGlobal should do anything to the browser. Can we make browserLoaded call _instantiateLazyBrowser explicitly?
Flags: needinfo?(dao+bmo)
Posted patch 1287330_patch_V1.diff (obsolete) — Splinter Review
Up to date with current mozilla-central + fix for browser_async_remove_tab.js test failure.

Added `ensureBrowserIsInserted` public method.
Attachment #8808138 - Attachment is obsolete: true
Attachment #8832976 - Flags: feedback?(dao+bmo)
Comment on attachment 8832976 [details] [diff] [review]
1287330_patch_V1.diff

(In reply to Kevin Jones from comment #174)
> Created attachment 8832976 [details] [diff] [review]
> 1287330_patch_V1.diff
> 
> Up to date with current mozilla-central + fix for
> browser_async_remove_tab.js test failure.
> 
> Added `ensureBrowserIsInserted` public method.

No need to make it "public" as long as it's only used by tests.

nit: indentation is somewhat messed up in this method.

Why didn't you just use _insertBrowser directly in BrowserTestUtils?
Attachment #8832976 - Flags: feedback?(dao+bmo) → feedback+
Posted patch 1287330_patch_V2.diff (obsolete) — Splinter Review
> Why didn't you just use _insertBrowser directly in BrowserTestUtils?

I thought I needed to expose a public method.
Attachment #8833031 - Flags: feedback?(dao+bmo)
Attachment #8832976 - Attachment is obsolete: true
Posted patch 1287330_patch_V3.diff (obsolete) — Splinter Review
Fixed indentation in `loadOneTab`.
Attachment #8833031 - Attachment is obsolete: true
Attachment #8833031 - Flags: feedback?(dao+bmo)
Attachment #8833034 - Flags: feedback?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #175)
> Comment on attachment 8832976 [details] [diff] [review]
> 1287330_patch_V1.diff
> 
> (In reply to Kevin Jones from comment #174)
> > Created attachment 8832976 [details] [diff] [review]
> > 1287330_patch_V1.diff
> > 
> > Up to date with current mozilla-central + fix for
> > browser_async_remove_tab.js test failure.
> > 
> > Added `ensureBrowserIsInserted` public method.
> 
> No need to make it "public" as long as it's only used by tests.
> 
> nit: indentation is somewhat messed up in this method.
> 
> Why didn't you just use _insertBrowser directly in BrowserTestUtils?

Dao, did you find indentation problem in `_insertBrowser` as well? (you referred to "this method")  If so, please specify.
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #178)
> Dao, did you find indentation problem in `_insertBrowser` as well?

no
Flags: needinfo?(dao+bmo)
Comment on attachment 8833034 [details] [diff] [review]
1287330_patch_V3.diff

How do Try results look for this?
Attachment #8833034 - Flags: feedback?(dao+bmo) → feedback+
(In reply to Dão Gottwald [:dao] from comment #180)
> Comment on attachment 8833034 [details] [diff] [review]
> 1287330_patch_V3.diff
> 
> How do Try results look for this?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a38ab53060dd485c71f1559b23992721075465a
I added `uriIsAboutBlank` to tab._browserParams, but it looks like I'm still getting it the old way in `_insertBrowser`.  I'll want to fix that.
I'm getting "invalid `in` operator" error for this code:

            if (this._browserBindingProperties[0] in browser) {
              for (let name of this._browserBindingProperties) {
                delete browser[name];
              }
            }

Do you know offhand what that could be about?  It seems the only way that could happen is if browser is not an object, but now the browser is a created and linked on tab creation, so there should always be a browser.
Flags: needinfo?(dao+bmo)
Posted patch 1287330_patch_V4.diff (obsolete) — Splinter Review
Apparently there are conditions where tab.linkedBrowser can be null when passed to `removeTab` (This condition exists without the patch applied).  I added a test for `tab.linkedBrowser` in `_insertBrowser`.

Also fixed `_insertBrowser` so it gets uriIsAboutBlank from tab._browserParams instead of generating it again.
Attachment #8833034 - Attachment is obsolete: true
Attachment #8833539 - Flags: feedback?(dao+bmo)
Attachment #8833539 - Flags: feedback?(dao+bmo) → feedback?
Attachment #8833539 - Flags: feedback?
(In reply to Kevin Jones from comment #186)
> Comment on attachment 8833539 [details] [diff] [review]
> 1287330_patch_V4.diff
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5497db753b7ddf309bec96cb82577a46141c11a3

Correction, this try is for a new patch that has not been uploaded yet.
Posted patch 1287330_patch_V7.diff (obsolete) — Splinter Review
In addition to comment 184:

Added `frameLoader` property to list of `_browserBindingProperties` to fix failure in browser_tabMatchesInAwesomebar.js.

BrowserTestUtils.js/browserLoaded:
Added test for browser.permanentKey to avoid trying to `_insetBrowser` on a browser that doesn't belong to a `tabbrowser-tab` in order to fix several test failures.

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be1585c0eb138a27ec97fd12f4580960df8f886
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce06a0c995f2bb26cb8016db820532d5cb72f0fe
Attachment #8833539 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Attachment #8833750 - Flags: feedback?(dao+bmo)
(In reply to Kevin Jones from comment #188)
> Created attachment 8833750 [details] [diff] [review]
> 1287330_patch_V7.diff
> 
> In addition to comment 184:
> 
> Added `frameLoader` property to list of `_browserBindingProperties` to fix
> failure in browser_tabMatchesInAwesomebar.js.

Note that frameLoader isn't a binding property, so at the very least this makes "_browserBindingProperties" misleading. Where is frameLoader used anyway such that browser_tabMatchesInAwesomebar.js breaks? I don't see it being used directly in the test.

> BrowserTestUtils.js/browserLoaded:
> Added test for browser.permanentKey to avoid trying to `_insetBrowser` on a
> browser that doesn't belong to a `tabbrowser-tab` in order to fix several
> test failures.

Can you handle tabbrowser.getTabForBrowser(browser) being null instead of the permanentKey check?
(In reply to Kevin Jones from comment #177)
> Created attachment 8833034 [details] [diff] [review]
> 1287330_patch_V3.diff
> 
> Fixed indentation in `loadOneTab`.

It's fine for an overly long variable name to break that indentation pattern. In other words the above change is unnecessary, please revert it.
(In reply to Dão Gottwald [:dao] from comment #189)
> > BrowserTestUtils.js/browserLoaded:
> > Added test for browser.permanentKey to avoid trying to `_insetBrowser` on a
> > browser that doesn't belong to a `tabbrowser-tab` in order to fix several
> > test failures.
> 
> Can you handle tabbrowser.getTabForBrowser(browser) being null instead of
> the permanentKey check?

In bug623683_window.xul[1] there is an odd condition where a non-tabbrowser browser is created, and it is named `gBrowser`.  So testing for `browser.ownerGlobal.gBrowser` returns true.  So I end up with code like:

let tabbrowser = browser.ownerGlobal.gBrowser;
if (tabbrowser && tabbrowser.getTabForBrowser) {
  tabbrowser._insertBrowser(tabbrowser.getTabForBrowser(browser));
}

So I used permanentKey test as it was more concise.  I'm happy to change it, just wondering if you can think of a more elegant test?

[1]https://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/bug263683_window.xul#48
(In reply to Dão Gottwald [:dao] from comment #190)
> (In reply to Kevin Jones from comment #177)
> > Created attachment 8833034 [details] [diff] [review]
> > 1287330_patch_V3.diff
> > 
> > Fixed indentation in `loadOneTab`.
> 
> It's fine for an overly long variable name to break that indentation
> pattern. In other words the above change is unnecessary, please revert it.

Okay.  In comment 178 you mention indentation that needed correcting.  I thought that was it.  AFAIK I haven't changed any other indentation, so I am wondering, to what were you referring in comment 178?
(In reply to Dão Gottwald [:dao] from comment #189)
> Note that frameLoader isn't a binding property, so at the very least this
> makes "_browserBindingProperties" misleading. Where is frameLoader used
> anyway such that browser_tabMatchesInAwesomebar.js breaks? I don't see it
> being used directly in the test.

browser_tabMatchesInAwesomebar.js[1] calls `gBrowser.updateBrowserRemoteness`[2] where `frameLoader.isFreshProcess` is accessed.

Do you think that `gBrowser.updateBrowserRemoteness` code should handle this differently?

[1]https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/urlbar/browser_tabMatchesInAwesomebar.js#86
[2]https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1709
Flags: needinfo?(dao+bmo)
If this bug means not creating the temporary non-remote xul:browser, but just the remote one and restore session there, this would save significant amount memory in parent browser when one has lots of non-restored tabs open.  And while saving memory, this would also improve GC time possibly significantly.
Currently I seem to have 381 extra JS globals in parent process because of non-restored tabs.

So, looking forward to see this landing :)
Posted patch 1287330_patch_V8.diff (obsolete) — Splinter Review
Revised BrowserTestUtils.js/browserLoaded - changed test for permanentKey to testing for `tabbrowser.getTabForBrowser(browser)`.  Note testing for `tabbrowser.getTabForBrowser` is first necessary due to peculiarity of bug623683_window.xul (see comment 191).

Revised tabbrowser.xul - removed "frameLoader" from list of `_browserBindingProperties`, and added test for `gBrowser.frameLoader` in `updateBrowserRemoteness`.
Attachment #8833750 - Attachment is obsolete: true
Attachment #8833750 - Flags: feedback?(dao+bmo)
Attachment #8836441 - Flags: feedback?(dao+bmo)
Posted patch 1287330_patch_V9.diff (obsolete) — Splinter Review
tabbrowser.xul/updateBrowserRemoteness: Instead of test for `aBrowser.frameLoader`, forced browser insertion code has been moved ahead of the "Abort if we're not going to change anything" test.
Attachment #8836442 - Flags: feedback?(dao+bmo)
Dao, since I hadn't heard from you I uploaded 2 prospective patches.  They both have been updated to replace the test for `browser.permanentKey` in BrowserTestUtils.js/browserLoaded for `tabbrowser.getTabForBrowser(browser)`

The difference between the two is the handling of `aBrowser.frameLoader` in tabbrowser.xul/updateBrowserRemoteness (needed to fix browser_tabMatchesInAwesomebar.js failure).  In patch V8, the test for "Abort if we're not going to change anything" is modified to return false if testing for `aBrowser.frameLoader.isFreshProcess` will cause a failure.  In patch V9, the code to force browser insertion was simply moved ahead of this test, thus forcing browser insertion no matter what.

The reason for the approach in patch V8 versus V9, is that the V8 approach attempts to keep from unnecessarily inserting the browser.  However you are probably more up to speed on how changing browser remoteness works, so please advise.
(In reply to Kevin Jones from comment #197)
> In patch V8, the test for
> "Abort if we're not going to change anything" is modified to return false if
> testing for `aBrowser.frameLoader.isFreshProcess` will cause a failure.

Clarification: the test returns true, causing the method to return false.
(In reply to Kevin Jones from comment #192)
> (In reply to Dão Gottwald [:dao] from comment #190)
> > (In reply to Kevin Jones from comment #177)
> > > Created attachment 8833034 [details] [diff] [review]
> > > 1287330_patch_V3.diff
> > > 
> > > Fixed indentation in `loadOneTab`.
> > 
> > It's fine for an overly long variable name to break that indentation
> > pattern. In other words the above change is unnecessary, please revert it.
> 
> Okay.  In comment 178 you mention indentation that needed correcting.  I
> thought that was it.  AFAIK I haven't changed any other indentation, so I am
> wondering, to what were you referring in comment 178?

I was referring to ensureBrowserIsInserted.
(In reply to Kevin Jones from comment #193)
> (In reply to Dão Gottwald [:dao] from comment #189)
> > Note that frameLoader isn't a binding property, so at the very least this
> > makes "_browserBindingProperties" misleading. Where is frameLoader used
> > anyway such that browser_tabMatchesInAwesomebar.js breaks? I don't see it
> > being used directly in the test.
> 
> browser_tabMatchesInAwesomebar.js[1] calls
> `gBrowser.updateBrowserRemoteness`[2] where `frameLoader.isFreshProcess` is
> accessed.
> 
> Do you think that `gBrowser.updateBrowserRemoteness` code should handle this
> differently?
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> urlbar/browser_tabMatchesInAwesomebar.js#86
> [2]https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#1709

Olli, can you comment on this? Should !aBrowser.frameLoader.isFreshProcess be considered true or false in updateBrowserRemoteness when there's no frameLoader yet?
Flags: needinfo?(dao+bmo) → needinfo?(bugs)
mystor ^
Flags: needinfo?(bugs) → needinfo?(michael)
(In reply to Dão Gottwald [:dao] from comment #200)
> Olli, can you comment on this? Should !aBrowser.frameLoader.isFreshProcess
> be considered true or false in updateBrowserRemoteness when there's no
> frameLoader yet?

I'm hoping to remove this flag entirely in bug 1338241. Until I land that, however, it should be false if we don't have a frameLoader yet.
Flags: needinfo?(michael)
(In reply to Michael Layzell [:mystor] from comment #202)
> (In reply to Dão Gottwald [:dao] from comment #200)
> > Olli, can you comment on this? Should !aBrowser.frameLoader.isFreshProcess
> > be considered true or false in updateBrowserRemoteness when there's no
> > frameLoader yet?
> 
> I'm hoping to remove this flag entirely in bug 1338241. Until I land that,
> however, it should be false if we don't have a frameLoader yet.

Hope I'm following the logic correctly - so then the updated `updateBrowserRemoteness` in https://bugzilla.mozilla.org/attachment.cgi?id=8836441&action=diff ( Lines 1704-1724) is correct the way it is?
Flags: needinfo?(dao+bmo)
To go a little deeper, I'm not that familiar with remote browser functionality, but I'm wondering if updating the browser remoteness even makes sense if the browser is lazy?

Like, maybe we should just be aborting right off the bat if the browser is lazy?
(In reply to Kevin Jones from comment #204)
> To go a little deeper, I'm not that familiar with remote browser
> functionality, but I'm wondering if updating the browser remoteness even
> makes sense if the browser is lazy?
> 
> Like, maybe we should just be aborting right off the bat if the browser is
> lazy?

We need to make sure we end up setting the right attributes, either immediately or when the lazy browser is finally inserted, but we shouldn't need to force the browser to be inserted. In practice, everything that happens in updateBrowserRemoteness that matters should automatically happen when the tab is restored, anyway. The only time it should really come into play immediately is when we're moving a tab from another window, and creating a new browser to swap frameloaders with the old one.
Posted patch 1287330_patch_V10.diff (obsolete) — Splinter Review
Update per comment 205.

Kris, is this something like you had in mind?

Should we return anything for lazy-browser condition?  It seems moot since anything testing for that requires a bound browser.
Attachment #8836442 - Attachment is obsolete: true
Attachment #8836442 - Flags: feedback?(dao+bmo)
Flags: needinfo?(kmaglione+bmo)
Attachment #8836701 - Flags: feedback?(dao+bmo)
(In reply to Kevin Jones from comment #206)
> Created attachment 8836701 [details] [diff] [review]
> 1287330_patch_V10.diff
> 
> Update per comment 205.
> 
> Kris, is this something like you had in mind?
> 
> Should we return anything for lazy-browser condition?  It seems moot since
> anything testing for that requires a bound browser.

This patch breaks browser_async_remove_tab.js test.  Maybe I haven't interpreted your comment correctly?
Comment on attachment 8836441 [details] [diff] [review]
1287330_patch_V8.diff

>+                (!aBrowser.frameLoader || !aBrowser.frameLoader.isFreshProcess)) {
>               return false;

If I understand mystor correctly, this should be:

(aBrowser.frameLoader && !aBrowser.frameLoader.isFreshProcess)
Flags: needinfo?(dao+bmo)
Attachment #8836441 - Flags: feedback?(dao+bmo)
Attachment #8836701 - Flags: feedback?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #208)
> Comment on attachment 8836441 [details] [diff] [review]
> 1287330_patch_V8.diff
> 
> >+                (!aBrowser.frameLoader || !aBrowser.frameLoader.isFreshProcess)) {
> >               return false;
> 
> If I understand mystor correctly, this should be:
> 
> (aBrowser.frameLoader && !aBrowser.frameLoader.isFreshProcess)

I think it's correct actually. What this code is checking is whether or not the browser remoteness update needs to occur. If this expression returns false, then the browser remoteness update needs to occur, and if it returns true, then it doesn't (assuming that the other parts also produce true).

We want to perform the update if both the frameloader exists, and it is a fresh process, which could be written as:

    !(aBrowser.frameLoader && aBrowser.frameLoader.isFreshProcess)

which is equivalent to the above expression.
Posted patch 1287330_patch_V11.diff (obsolete) — Splinter Review
Slight revision, I used mystor's suggestion as the syntax seems more intuitive to me.

try: https://mail.google.com/mail/u/0/?ui=2&view=btop&ver=1wp7hm5bkg3kc&search=inbox&th=15a3f6cecdb07412&cvid=3

Dao, do you think we're now on the right track here?
Attachment #8836441 - Attachment is obsolete: true
Attachment #8836701 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Comment on attachment 8837388 [details] [diff] [review]
1287330_patch_V11.diff

Looks alright to me.

>             if (arguments.length == 2 &&
>                 typeof arguments[1] == "object" &&
>                 !(arguments[1] instanceof Ci.nsIURI)) {
>               let params = arguments[1];
>-              aTriggeringPrincipal  = params.triggeringPrincipal
>-              aReferrerURI          = params.referrerURI;
>-              aReferrerPolicy       = params.referrerPolicy;
>-              aCharset              = params.charset;
>-              aPostData             = params.postData;
>-              aLoadInBackground     = params.inBackground;
>-              aAllowThirdPartyFixup = params.allowThirdPartyFixup;
>-              aFromExternal         = params.fromExternal;
>-              aRelatedToCurrent     = params.relatedToCurrent;
>-              aAllowMixedContent    = params.allowMixedContent;
>-              aSkipAnimation        = params.skipAnimation;
>-              aForceNotRemote       = params.forceNotRemote;
>-              aPreferredRemoteType  = params.preferredRemoteType;
>-              aNoReferrer           = params.noReferrer;
>-              aUserContextId        = params.userContextId;
>-              aRelatedBrowser       = params.relatedBrowser;
>-              aOriginPrincipal      = params.originPrincipal;
>-              aOpener               = params.opener;
>-              aIsPrerendered        = params.isPrerendered;
>+              aTriggeringPrincipal   = params.triggeringPrincipal
>+              aReferrerURI           = params.referrerURI;
>+              aReferrerPolicy        = params.referrerPolicy;
>+              aCharset               = params.charset;
>+              aPostData              = params.postData;
>+              aLoadInBackground      = params.inBackground;
>+              aAllowThirdPartyFixup  = params.allowThirdPartyFixup;
>+              aFromExternal          = params.fromExternal;
>+              aRelatedToCurrent      = params.relatedToCurrent;
>+              aAllowMixedContent     = params.allowMixedContent;
>+              aSkipAnimation         = params.skipAnimation;
>+              aForceNotRemote        = params.forceNotRemote;
>+              aPreferredRemoteType   = params.preferredRemoteType;
>+              aNoReferrer            = params.noReferrer;
>+              aUserContextId         = params.userContextId;
>+              aRelatedBrowser        = params.relatedBrowser;
>+              aOriginPrincipal       = params.originPrincipal;
>+              aOpener                = params.opener;
>+              aIsPrerendered         = params.isPrerendered;

Again, please revert this.
Flags: needinfo?(dao+bmo)
Posted patch 1287330_patch_V12.diff (obsolete) — Splinter Review
`loadOneTab` args reverted
Attachment #8837388 - Attachment is obsolete: true
Attachment #8837591 - Flags: review?(dao+bmo)
How do try results look for the latest patch? The link from comment 210 isn't the right one.
(In reply to Dão Gottwald [:dao] from comment #213)
> How do try results look for the latest patch? The link from comment 210
> isn't the right one.

Sorry:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=145d1a911c079c338771d08c77343b3a30b9d76d&selectedJob=77430567
Have you looked into the browser_verify_content_about_newtab.js failure?
(In reply to Dão Gottwald [:dao] from comment #215)
> Have you looked into the browser_verify_content_about_newtab.js failure?

I ignored it, as the failure didn't seem like it could be a result of anything in the patch (document.getElementById(...) is null).

But I'll give it a closer look.
(In reply to Kevin Jones from comment #216)
> (In reply to Dão Gottwald [:dao] from comment #215)
> > Have you looked into the browser_verify_content_about_newtab.js failure?
> 
> I ignored it, as the failure didn't seem like it could be a result of
> anything in the patch (document.getElementById(...) is null).
> 
> But I'll give it a closer look.

So far I have been unable to reproduce, but I can keep trying.
(In reply to Kevin Jones from comment #216)
> (In reply to Dão Gottwald [:dao] from comment #215)
> > Have you looked into the browser_verify_content_about_newtab.js failure?
> 
> I ignored it, as the failure didn't seem like it could be a result of
> anything in the patch (document.getElementById(...) is null).

It's possible that you're working with a revision where the test was broken, but if you don't see the failure on https://treeherder.mozilla.org/#/jobs?repo=mozilla-central, rebase your clone to mozilla-central tip, and still get the failure on Try, the it's caused by your patch.
(In reply to Dão Gottwald [:dao] from comment #218)
> (In reply to Kevin Jones from comment #216)
> > (In reply to Dão Gottwald [:dao] from comment #215)
> > > Have you looked into the browser_verify_content_about_newtab.js failure?
> > 
> > I ignored it, as the failure didn't seem like it could be a result of
> > anything in the patch (document.getElementById(...) is null).
> 
> It's possible that you're working with a revision where the test was broken,
> but if you don't see the failure on
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-central, rebase your
> clone to mozilla-central tip, and still get the failure on Try, the it's
> caused by your patch.

This is what I'm getting now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b5e3557b92b189d30cbb1d8849bfcf539c6c1b&selectedJob=77700498

I tried once again before rebasing and I got clean results:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b764a9b8bb5b3ae3822630aaf089652cfd01021&selectedJob=77671595

I didn't have any problems applying the patch on the latest mozilla-central.  Do you think that the new failures can be a result of the patch?
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #219)
> Do you think that the new failures can be a result of the patch?

Probably... note that browser_tab_drag_drop_perwindow.js was modified a month ago along with tabbrowser.xml, which might be related:
https://hg.mozilla.org/mozilla-central/rev/dc76b4cad2f9
Flags: needinfo?(dao+bmo)
Fixes browser_tab_drag_drop_perwindow.js failure, which was failing because `aBrowser.frameLoader.isFreshProcess` was called on a frameLoader which was sometimes null in `updateBrowserRemotenessByURL`.  Tests for `aBrowser.frameLoader == null` in which case passes along to `updateBrowserRemoteness` where it is dealt with there.

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

browser_windowopen_reflows.js bustage also appears in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central.
Attachment #8837591 - Attachment is obsolete: true
Attachment #8837591 - Flags: review?(dao+bmo)
Flags: needinfo?(dao+bmo)
Looks good!
Flags: needinfo?(dao+bmo)
Attachment #8838416 - Flags: review?(dao+bmo)
I wonder if we should invert forceBrowserInsertion and only use the lazy browser when the caller opts in. E.g. session restore could explicitly do this, not sure if there are other cases we care about. Something to think about ... changing this doesn't necessarily have to block landing this patch.
Okay.  I'm all for getting this landed and investigating that in a separate bug.
Personally I think I like the default lazy-browser paradigm better.  Later bugs will be doing all they can to keep the browser lazy where possible.

But I am most happy to concede.
Comment on attachment 8838416 [details] [diff] [review]
1287330_patch_rebased2_V3.diff

Thanks for your persistent and hard work on this! Let's hope the patch sticks this time...
Attachment #8838416 - Flags: review?(dao+bmo) → review+
Oops:

applying https://bug1287330.bmoattachments.org/attachment.cgi?id=8838416
patching file browser/base/content/tabbrowser.xml
Hunk #1 FAILED at 1508
Hunk #2 FAILED at 1531
Hunk #3 FAILED at 1550
Hunk #7 FAILED at 2034
Hunk #9 FAILED at 2156
Hunk #10 FAILED at 2181
Hunk #11 FAILED at 2243
7 out of 15 hunks FAILED -- saving rejects to file browser/base/content/tabbrowser.xml.rej
abort: patch failed to apply
https://hg.mozilla.org/mozilla-central/rev/a273874c2205 landed in the meantime, so this needs another update.
(In reply to Dão Gottwald [:dao] from comment #228)
> https://hg.mozilla.org/mozilla-central/rev/a273874c2205 landed in the
> meantime, so this needs another update.

Okay, rebasing...
(In reply to Dão Gottwald [:dao] from comment #226)
> Comment on attachment 8838416 [details] [diff] [review]
> 1287330_patch_rebased2_V3.diff
> 
> Thanks for your persistent and hard work on this! Let's hope the patch
> sticks this time...

Very welcome :-)  I'd like to see this happen.
Rebased
Attachment #8838416 - Attachment is obsolete: true
Attachment #8838603 - Flags: review?(dao+bmo)
Oops, I'm not handling aSameProcessAsFrameLoader properly.
(In reply to Kevin Jones from comment #233)
> Oops, I'm not handling aSameProcessAsFrameLoader properly.

I cancelled the remaining Try jobs.
This one should work now.  Very sorry for making you do additional work.
Attachment #8838603 - Attachment is obsolete: true
Attachment #8838637 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #234)
> (In reply to Kevin Jones from comment #233)
> > Oops, I'm not handling aSameProcessAsFrameLoader properly.
> 
> I cancelled the remaining Try jobs.

I tried to cancel them, but I'm having trouble logging into LDAP.  Working on that now.
Try looks green enough so far, results are complete on Linux, so I'm going to attempt to land this.
Luck is not on your side today. Bug 1338241 obsoleted your patch again.
Flags: needinfo?(kevinhowjones)
rebased and updated.

Test for aBrowser "remoteType" attribute instead of property in `updateBrowserRemotenessByURL` to accommodate lazy browsers.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=031cc7b5b8d8b87b794e6ba454b3cb2c7018aaf7

https://treeherder.mozilla.org/#/jobs?repo=try&revision=205e49c40e9906da96587893659c0d84413e5aea
Attachment #8838637 - Attachment is obsolete: true
Flags: needinfo?(kevinhowjones)
Comment on attachment 8839035 [details] [diff] [review]
1287330_patch_rebased4_V7.diff

Dao, the test failures I see I am unable to duplicate.  Also, the failures are not consistent between the two try runs.

Any thoughts?
Flags: needinfo?(dao+bmo)
Comment on attachment 8839145 [details] [diff] [review]
1287330_patch_rebased5_V1.diff

this appears to be identical to attachment 8839035 [details] [diff] [review]
Attachment #8839145 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #243)
> Comment on attachment 8839145 [details] [diff] [review]
> 1287330_patch_rebased5_V1.diff
> 
> this appears to be identical to attachment 8839035 [details] [diff] [review]

Yes, I pulled the lastest MC, but apparently there weren't any changes that affected the patch.
I am seeing failures, but none of the ones from before.  Neither can I duplicate any of them locally on debug builds.
Comment on attachment 8839035 [details] [diff] [review]
1287330_patch_rebased4_V7.diff

Fresh try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01800b0558651749468a6a94d674446fab1cb766

This looks good.
Flags: needinfo?(dao+bmo)
Attachment #8839035 - Flags: review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0709404b9ae
Insert tabs' linkedBrowser lazily into the document. r=dao
Blocks: 1341474
(In reply to Pulsebot from comment #247)
> Pushed by dgottwald@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e0709404b9ae
> Insert tabs' linkedBrowser lazily into the document. r=dao

fingers crossed...
https://hg.mozilla.org/mozilla-central/rev/e0709404b9ae
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
No longer blocks: 1341474
It would be really nice to have this fixed on Android, too. And probably much easier, given the smaller number of consumers of that tabs API. It's been one of my worst Fennec pain points lately...
Flags: needinfo?(kmaglione+bmo)
Blocks: 1343090
No longer blocks: 1363240
Depends on: 1363240
No longer blocks: 1436351
No longer depends on: 1436822
You need to log in before you can comment on or make changes to this bug.