Closed
Bug 1287330
Opened 8 years ago
Closed 8 years ago
Insert tabs' linkedBrowser lazily into the document
Categories
(Firefox :: Tabbed Browser, enhancement, P2)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: u462496, Assigned: u462496)
References
Details
Attachments
(1 file, 47 obsolete files)
(deleted),
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.
Attachment #8771780 -
Flags: feedback?(dao+bmo)
Attachment #8771780 -
Attachment is obsolete: true
Attachment #8771780 -
Flags: feedback?(dao+bmo)
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)
Attachment #8772003 -
Attachment is obsolete: true
Attachment #8772003 -
Flags: feedback?(dao+bmo)
Attachment #8772005 -
Flags: feedback?(dao+bmo)
Comment 4•8 years ago
|
||
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-
Comment 5•8 years ago
|
||
(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.
Updated•8 years ago
|
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 8•8 years ago
|
||
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);
Comment 10•8 years ago
|
||
Yes, but the property should be private (_browserParams) and the URI should be part of it.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 11•8 years ago
|
||
(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?
Assignee | ||
Comment 12•8 years ago
|
||
ie:
<method name="_linkBrowserToTab">
<parameter name="aTab"/>
<body>
<![CDATA[
"use strict";
let params = aTab._browserParams;
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
The proxy should be overwritten by the real browser, and at that point recursion shouldn't be possible anymore.
Assignee | ||
Comment 16•8 years ago
|
||
(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)
Assignee | ||
Comment 17•8 years ago
|
||
I wonder if it is a `this
Assignee | ||
Comment 18•8 years ago
|
||
I wonder if it is a `this` issue.
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
I can't seem to reproduce the issue. What are the exact steps to do so?
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 22•8 years ago
|
||
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.
Assignee | ||
Comment 23•8 years ago
|
||
...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
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
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-
Comment 26•8 years ago
|
||
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
Assignee | ||
Comment 27•8 years ago
|
||
(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?
Comment 28•8 years ago
|
||
./mach mochitest --browser-chrome browser/base/content/test/general/ is all I did.
Assignee | ||
Comment 29•8 years ago
|
||
(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.
Assignee | ||
Comment 30•8 years ago
|
||
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.
Comment 31•8 years ago
|
||
(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
Comment 32•8 years ago
|
||
(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.
Assignee | ||
Comment 33•8 years ago
|
||
Attachment #8774129 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 34•8 years ago
|
||
> 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.
Assignee | ||
Comment 35•8 years ago
|
||
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 36•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8773739 -
Attachment is obsolete: true
Assignee | ||
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
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)
Assignee | ||
Comment 39•8 years ago
|
||
(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`?
Comment 40•8 years ago
|
||
(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.
Assignee | ||
Comment 41•8 years ago
|
||
Attachment #8774344 -
Attachment is obsolete: true
Attachment #8774435 -
Flags: review?(dao+bmo)
Comment 42•8 years ago
|
||
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
Assignee | ||
Comment 43•8 years ago
|
||
Attachment #8774435 -
Attachment is obsolete: true
Attachment #8774435 -
Flags: review?(dao+bmo)
Attachment #8774449 -
Flags: review?(dao+bmo)
Comment 44•8 years ago
|
||
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+
Assignee | ||
Comment 45•8 years ago
|
||
Here you go :-)
Attachment #8774449 -
Attachment is obsolete: true
Comment 46•8 years ago
|
||
Lots of failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ee950aa12c
Assignee | ||
Comment 47•8 years ago
|
||
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.
Comment 48•8 years ago
|
||
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.
Assignee | ||
Comment 49•8 years ago
|
||
(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?
Comment 50•8 years ago
|
||
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?
Assignee | ||
Comment 51•8 years ago
|
||
(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 :-)
Assignee | ||
Comment 52•8 years ago
|
||
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.
Assignee | ||
Comment 53•8 years ago
|
||
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.
Assignee | ||
Comment 54•8 years ago
|
||
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.
Comment 55•8 years ago
|
||
(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.
Assignee | ||
Comment 56•8 years ago
|
||
(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.
Assignee | ||
Comment 57•8 years ago
|
||
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)
--------------------------------------------------
Assignee | ||
Comment 58•8 years ago
|
||
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.
Assignee | ||
Comment 59•8 years ago
|
||
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.
Assignee | ||
Comment 60•8 years ago
|
||
Remaining tests which still need investigation, apparently having a problem with the proxy:
browser_history_persist.js
browser_passwordmgr_switchtab.js
browser_bug817947.js
Assignee | ||
Comment 61•8 years ago
|
||
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.
Assignee | ||
Comment 62•8 years ago
|
||
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.
Assignee | ||
Comment 63•8 years ago
|
||
Attachment #8776828 -
Flags: feedback?(dao+bmo)
Comment 64•8 years ago
|
||
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+
Assignee | ||
Comment 65•8 years ago
|
||
(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.
Comment 66•8 years ago
|
||
(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...
Assignee | ||
Comment 67•8 years ago
|
||
(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.
Assignee | ||
Comment 68•8 years ago
|
||
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
Assignee | ||
Comment 69•8 years ago
|
||
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
Assignee | ||
Comment 70•8 years ago
|
||
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 | ||
Comment 71•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8777237 -
Attachment is patch: true
Comment 72•8 years ago
|
||
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+
Assignee | ||
Comment 73•8 years ago
|
||
(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.
Assignee | ||
Comment 74•8 years ago
|
||
> _browserPropertyNamesList should be renamed such that it's clear that these properties
> need the browser to be in the DOM.
How about _browserXBLPropertyNames?
Comment 75•8 years ago
|
||
Well, what exactly is this array? Does it contain all XBL properties and nothing else?
Assignee | ||
Comment 76•8 years ago
|
||
(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.
Comment 77•8 years ago
|
||
Then that seems fine, although I'd call it _browserBindingProperties.
Assignee | ||
Comment 78•8 years ago
|
||
> 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`
Comment 79•8 years ago
|
||
You can also just check aTab.linkedPanel.
Assignee | ||
Comment 80•8 years ago
|
||
(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.
Assignee | ||
Comment 81•8 years ago
|
||
(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?
Comment 82•8 years ago
|
||
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.
Assignee | ||
Comment 83•8 years ago
|
||
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.
Assignee | ||
Comment 84•8 years ago
|
||
Attachment #8777237 -
Attachment is obsolete: true
Attachment #8777300 -
Flags: review?(dao+bmo)
Comment 85•8 years ago
|
||
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)
Assignee | ||
Comment 86•8 years ago
|
||
(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.)
Assignee | ||
Comment 87•8 years ago
|
||
(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.
Comment 88•8 years ago
|
||
(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?
Assignee | ||
Comment 89•8 years ago
|
||
(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.
Comment 90•8 years ago
|
||
delete should be extremely cheap, but I suppose you could still wrap it in a check like if (this._browserBindingProperties[0] in browser) {...}.
Assignee | ||
Comment 91•8 years ago
|
||
Attachment #8777300 -
Attachment is obsolete: true
Attachment #8777342 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 92•8 years ago
|
||
A little more clean-up.
Attachment #8777342 -
Attachment is obsolete: true
Attachment #8777342 -
Flags: review?(dao+bmo)
Attachment #8777347 -
Flags: review?(dao+bmo)
Comment 93•8 years ago
|
||
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+
Comment 94•8 years ago
|
||
webProgress and permitUnload is missing from _browserBindingProperties:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=242fe74ca751
Assignee | ||
Comment 95•8 years ago
|
||
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)
Assignee | ||
Comment 96•8 years ago
|
||
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)
Assignee | ||
Comment 97•8 years ago
|
||
Assignee | ||
Comment 98•8 years ago
|
||
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.
Assignee | ||
Comment 99•8 years ago
|
||
Removed unnecessary semi-colon in _createLazyBrowser.
Attachment #8777846 -
Attachment is obsolete: true
Attachment #8777846 -
Flags: review?(dao+bmo)
Attachment #8778304 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 100•8 years ago
|
||
Do you think we should change the name of `TabBrowserCreated` event?
Comment 101•8 years ago
|
||
Yeah, probably TabBrowserInserted.
Comment 102•8 years ago
|
||
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.
Assignee | ||
Comment 103•8 years ago
|
||
Okay, new patch coming right up.
Assignee | ||
Comment 104•8 years ago
|
||
Here it is
Attachment #8778304 -
Attachment is obsolete: true
Attachment #8778304 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 105•8 years ago
|
||
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
Assignee | ||
Comment 106•8 years ago
|
||
Try this
Assignee | ||
Comment 107•8 years ago
|
||
Attachment #8778427 -
Attachment is obsolete: true
Assignee | ||
Comment 108•8 years ago
|
||
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)
Assignee | ||
Comment 109•8 years ago
|
||
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)
Assignee | ||
Comment 110•8 years ago
|
||
(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.
Assignee | ||
Comment 111•8 years ago
|
||
Comment 112•8 years ago
|
||
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+
Assignee | ||
Comment 113•8 years ago
|
||
Changed comments
Attachment #8782630 -
Attachment is obsolete: true
Attachment #8782879 -
Flags: review?(dao+bmo)
Comment 114•8 years ago
|
||
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+
Updated•8 years ago
|
Summary: Lazy-browser Tabs - add proxy for linkedBrowser → Insert tabs' linkedBrowser lazily into the document
Comment 115•8 years ago
|
||
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.
Comment 116•8 years ago
|
||
(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
Assignee | ||
Comment 117•8 years ago
|
||
(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?
Comment 118•8 years ago
|
||
I think you can pull from inbound in central, see hg pull --help
Assignee | ||
Comment 119•8 years ago
|
||
(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?
Assignee | ||
Comment 120•8 years ago
|
||
Never mind, I went for it.
Assignee | ||
Comment 121•8 years ago
|
||
Try this one...
Attachment #8782879 -
Attachment is obsolete: true
Attachment #8782915 -
Flags: review?(dao+bmo)
Updated•8 years ago
|
Attachment #8782915 -
Flags: review?(dao+bmo) → review+
Comment 122•8 years ago
|
||
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/865501b808e3
Insert tabs' linkedBrowser lazily into the document. r=dao
Comment 123•8 years ago
|
||
bugherder |
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 124•8 years ago
|
||
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 → ---
Assignee | ||
Comment 125•8 years ago
|
||
(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)
Assignee | ||
Comment 126•8 years ago
|
||
(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
Comment 127•8 years ago
|
||
(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)
Assignee | ||
Comment 128•8 years ago
|
||
(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?
Assignee | ||
Comment 129•8 years ago
|
||
> Any suggestions on how to troubleshoot?
I observe that when it does fail, it times out after all of the tests are completed successfully.
Comment 130•8 years ago
|
||
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.
Assignee | ||
Comment 131•8 years ago
|
||
(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.
Assignee | ||
Comment 132•8 years ago
|
||
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)
Assignee | ||
Comment 133•8 years ago
|
||
Assignee | ||
Comment 134•8 years ago
|
||
Attachment #8783360 -
Attachment is obsolete: true
Attachment #8783360 -
Flags: feedback?(kmaglione+bmo)
Attachment #8783360 -
Flags: feedback?(dao+bmo)
Comment 135•8 years ago
|
||
Merge of backout: https://hg.mozilla.org/mozilla-central/rev/7a646dca439b
status-firefox51:
fixed → ---
Target Milestone: Firefox 51 → ---
Assignee | ||
Comment 136•8 years ago
|
||
Updated patch includes fix for bug 1297005, and (hopefully) fix for test failure in browser_async_remove_tab.js (comment 124).
Assignee | ||
Comment 137•8 years ago
|
||
Assignee | ||
Comment 138•8 years ago
|
||
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 139•8 years ago
|
||
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+
Assignee | ||
Comment 140•8 years ago
|
||
(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)
Comment 141•8 years ago
|
||
_beginRemoveTab has already been called in that case (hence aTab.closing).
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 142•8 years ago
|
||
Okay.
Comment 143•8 years ago
|
||
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
Comment 145•8 years ago
|
||
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)
Assignee | ||
Comment 146•8 years ago
|
||
(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)
Assignee | ||
Comment 147•8 years ago
|
||
(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.
Assignee | ||
Comment 148•8 years ago
|
||
WIP patch uploaded for record-keeping purposes
Comment 149•8 years ago
|
||
(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)
Assignee | ||
Comment 150•8 years ago
|
||
(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)
Assignee | ||
Comment 151•8 years ago
|
||
(In reply to Allasso Travesser from comment #150)
> all of the addTask tests pass okay
Should be `add_task`.
Assignee | ||
Comment 152•8 years ago
|
||
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)
Assignee | ||
Comment 153•8 years ago
|
||
Allasso Travesser is now Kevin Jones.
Comment 154•8 years ago
|
||
Attachment #8783682 -
Attachment is obsolete: true
Attachment #8805734 -
Attachment is obsolete: true
Assignee | ||
Comment 155•8 years ago
|
||
(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?
Comment 156•8 years ago
|
||
(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)
Comment 157•8 years ago
|
||
> 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
Assignee | ||
Comment 158•8 years ago
|
||
(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.
Assignee | ||
Comment 159•8 years ago
|
||
(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)
Comment 160•8 years ago
|
||
(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)
Assignee | ||
Comment 161•8 years ago
|
||
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)
Assignee | ||
Comment 162•8 years ago
|
||
needinfo'ing David regarding comment 161
Flags: needinfo?(dteller)
Comment 163•8 years ago
|
||
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?
Assignee | ||
Comment 164•8 years ago
|
||
(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.
Comment 165•8 years ago
|
||
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?
Assignee | ||
Comment 166•8 years ago
|
||
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?
Comment 167•8 years ago
|
||
(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.
Comment 168•8 years ago
|
||
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.
Comment 169•8 years ago
|
||
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)
Assignee | ||
Comment 170•8 years ago
|
||
(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.
Assignee | ||
Comment 171•8 years ago
|
||
(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.
Assignee | ||
Comment 172•8 years ago
|
||
(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)
Comment 173•8 years ago
|
||
I don't think accessing ownerDocument / ownerGlobal should do anything to the browser. Can we make browserLoaded call _instantiateLazyBrowser explicitly?
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 174•8 years ago
|
||
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 175•8 years ago
|
||
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+
Assignee | ||
Comment 176•8 years ago
|
||
> 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
Assignee | ||
Comment 177•8 years ago
|
||
Fixed indentation in `loadOneTab`.
Attachment #8833031 -
Attachment is obsolete: true
Attachment #8833031 -
Flags: feedback?(dao+bmo)
Attachment #8833034 -
Flags: feedback?(dao+bmo)
Assignee | ||
Comment 178•8 years ago
|
||
(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)
Comment 179•8 years ago
|
||
(In reply to Kevin Jones from comment #178)
> Dao, did you find indentation problem in `_insertBrowser` as well?
no
Flags: needinfo?(dao+bmo)
Comment 180•8 years ago
|
||
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+
Assignee | ||
Comment 181•8 years ago
|
||
(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
Assignee | ||
Comment 182•8 years ago
|
||
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.
Assignee | ||
Comment 183•8 years ago
|
||
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)
Assignee | ||
Comment 184•8 years ago
|
||
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)
Assignee | ||
Comment 185•8 years ago
|
||
Comment on attachment 8833539 [details] [diff] [review]
1287330_patch_V4.diff
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b048b72cc2abac1902c00eb8c62837fa3001324b
Attachment #8833539 -
Flags: feedback?(dao+bmo) → feedback?
Attachment #8833539 -
Flags: feedback?
Assignee | ||
Comment 186•8 years ago
|
||
Comment on attachment 8833539 [details] [diff] [review]
1287330_patch_V4.diff
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5497db753b7ddf309bec96cb82577a46141c11a3
Assignee | ||
Comment 187•8 years ago
|
||
(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.
Assignee | ||
Comment 188•8 years ago
|
||
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)
Comment 189•8 years ago
|
||
(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?
Comment 190•8 years ago
|
||
(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.
Assignee | ||
Comment 191•8 years ago
|
||
(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
Assignee | ||
Comment 192•8 years ago
|
||
(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?
Assignee | ||
Comment 193•8 years ago
|
||
(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)
Comment 194•8 years ago
|
||
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 :)
Assignee | ||
Comment 195•8 years ago
|
||
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)
Assignee | ||
Comment 196•8 years ago
|
||
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)
Assignee | ||
Comment 197•8 years ago
|
||
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.
Assignee | ||
Comment 198•8 years ago
|
||
(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.
Comment 199•8 years ago
|
||
(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.
Comment 200•8 years ago
|
||
(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)
Comment 202•8 years ago
|
||
(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)
Assignee | ||
Comment 203•8 years ago
|
||
(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)
Assignee | ||
Comment 204•8 years ago
|
||
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?
Comment 205•8 years ago
|
||
(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.
Assignee | ||
Comment 206•8 years ago
|
||
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)
Assignee | ||
Comment 207•8 years ago
|
||
(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 208•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8836701 -
Flags: feedback?(dao+bmo)
Comment 209•8 years ago
|
||
(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.
Assignee | ||
Comment 210•8 years ago
|
||
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 211•8 years ago
|
||
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)
Assignee | ||
Comment 212•8 years ago
|
||
`loadOneTab` args reverted
Attachment #8837388 -
Attachment is obsolete: true
Attachment #8837591 -
Flags: review?(dao+bmo)
Comment 213•8 years ago
|
||
How do try results look for the latest patch? The link from comment 210 isn't the right one.
Assignee | ||
Comment 214•8 years ago
|
||
(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
Comment 215•8 years ago
|
||
Have you looked into the browser_verify_content_about_newtab.js failure?
Assignee | ||
Comment 216•8 years ago
|
||
(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.
Assignee | ||
Comment 217•8 years ago
|
||
(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.
Comment 218•8 years ago
|
||
(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.
Assignee | ||
Comment 219•8 years ago
|
||
(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)
Comment 220•8 years ago
|
||
(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)
Assignee | ||
Comment 221•8 years ago
|
||
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)
Attachment #8838416 -
Flags: review?(dao+bmo)
Comment 223•8 years ago
|
||
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.
Assignee | ||
Comment 224•8 years ago
|
||
Okay. I'm all for getting this landed and investigating that in a separate bug.
Assignee | ||
Comment 225•8 years ago
|
||
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 226•8 years ago
|
||
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+
Comment 227•8 years ago
|
||
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
Comment 228•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a273874c2205 landed in the meantime, so this needs another update.
Assignee | ||
Comment 229•8 years ago
|
||
(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...
Assignee | ||
Comment 230•8 years ago
|
||
(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.
Assignee | ||
Comment 231•8 years ago
|
||
Rebased
Attachment #8838416 -
Attachment is obsolete: true
Attachment #8838603 -
Flags: review?(dao+bmo)
Comment 232•8 years ago
|
||
Comment on attachment 8838603 [details] [diff] [review]
1287330_patch_rebased3_V1.diff
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b55178d66200445a0f84f50c6e05d029c3cf84f
Attachment #8838603 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Comment 233•8 years ago
|
||
Oops, I'm not handling aSameProcessAsFrameLoader properly.
Comment 234•8 years ago
|
||
(In reply to Kevin Jones from comment #233)
> Oops, I'm not handling aSameProcessAsFrameLoader properly.
I cancelled the remaining Try jobs.
Assignee | ||
Comment 235•8 years ago
|
||
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)
Comment 236•8 years ago
|
||
Comment on attachment 8838637 [details] [diff] [review]
1287330_patch_rebased3_V2.diff
https://treeherder.mozilla.org/#/jobs?repo=try&revision=897b08b4f6edf7edacca92f7a35901cadbdb7e7b
Attachment #8838637 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Comment 237•8 years ago
|
||
(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.
Comment 238•8 years ago
|
||
Try looks green enough so far, results are complete on Linux, so I'm going to attempt to land this.
Comment 239•8 years ago
|
||
Luck is not on your side today. Bug 1338241 obsoleted your patch again.
Flags: needinfo?(kevinhowjones)
Assignee | ||
Comment 240•8 years ago
|
||
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)
Assignee | ||
Comment 241•8 years ago
|
||
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)
Assignee | ||
Comment 242•8 years ago
|
||
Comment 243•8 years ago
|
||
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
Assignee | ||
Comment 244•8 years ago
|
||
(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.
Assignee | ||
Comment 245•8 years ago
|
||
I am seeing failures, but none of the ones from before. Neither can I duplicate any of them locally on debug builds.
Comment 246•8 years ago
|
||
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+
Comment 247•8 years ago
|
||
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0709404b9ae
Insert tabs' linkedBrowser lazily into the document. r=dao
Assignee | ||
Comment 248•8 years ago
|
||
(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...
Comment 249•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 250•8 years ago
|
||
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)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•