Add discardBrowser method to tabbrowser.xml

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: kernp25, Assigned: u462496)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

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

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 17 obsolete attachments)

12.38 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
WebExtensions can make use of it too.

e.g. browser.tabs.unload()

Like it does with browser.tabs.remove() also calling gBrowser.removeTab
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#546
Depends on: 906076
I think that if we were going to support this in WebExtensions, it would make more sense as a tab attribute, that could be queried and changed via tabs.update. I'm not sure that "unload" is the right name, though. But I can't immediately think of a better one, either.
(In reply to Kris Maglione [:kmag] from comment #1)
> I think that if we were going to support this in WebExtensions, it would
> make more sense as a tab attribute, that could be queried and changed via
> tabs.update. I'm not sure that "unload" is the right name, though. But I
> can't immediately think of a better one, either.

From the report and from looking around the web, it seems chrome has experimental support for this, and I assumed this is the API they provide. Isn't it?
Flags: needinfo?(kmaglione+bmo)
Not as far as I know. It's not documented, anyway, and it's not part of the tabs API schema in the Chromium repo.
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Comment 4

2 years ago
Created attachment 8768472 [details] [diff] [review]
bug-1284886-patch_1.diff
(Reporter)

Comment 5

2 years ago
I'm using this code in my add-on and it's working fine.

https://addons.mozilla.org/en-US/firefox/addon/auto-unload-tab/
(Reporter)

Updated

2 years ago
Attachment #8768472 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8768472 [details] [diff] [review]
bug-1284886-patch_1.diff

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

I'm not the best person to review this. mconley would be a better choice. I'm a bit worried that the implementation is going to cause trouble, though. Creating browsers that look like tabs but aren't is going to break code that assumes that they are.

::: browser/base/content/tabbrowser.xml
@@ +2775,5 @@
> +
> +	  <method name="unloadTab">
> +        <parameter name="aTab"/>
> +        <body>
> +          <![CDATA[

What happens if the tab is currently active?

@@ +2778,5 @@
> +        <body>
> +          <![CDATA[
> +		    let tabState = SessionStore.getTabState(aTab);
> +		    let browser = aTab.linkedBrowser;
> +		    let remote = gMultiProcessBrowser &&

Just `browser.isRemoteBrowser` is enough.

@@ +2781,5 @@
> +		    let browser = aTab.linkedBrowser;
> +		    let remote = gMultiProcessBrowser &&
> +                         browser.isRemoteBrowser &&
> +                         E10SUtils.canLoadURIInProcess(null, Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);
> +			

There are a lot of white space issues here. Please make sure that you expand tabs to spaces, use 2-space indentation, and remove white space from empty lines.

@@ +2787,5 @@
> +            let temp = this._createBrowser({remote: remote, uriIsAboutBlank: true});
> +    
> +            let notificationbox = this.getNotificationBox(temp);
> +            let tabPanel = this.mPanelContainer;
> +            if (!notificationbox.parentNode) {

This check is not necessary.

@@ +2799,5 @@
> +      
> +            temp.docShellIsActive = false;
> +    
> +            if (!remote) {
> +              this._outerWindowIDBrowserMap.set(temp.outerWindowID, temp);

This is not necessary.

@@ +2804,5 @@
> +            }
> +    
> +            this._swapBrowserDocShells(aTab, temp);
> +      
> +	        if (this._outerWindowIDBrowserMap.has(temp.outerWindowID)) {

This check is not necessary.

@@ +2809,5 @@
> +              this._outerWindowIDBrowserMap.delete(temp.outerWindowID);
> +            }
> +			
> +            temp.destroy();
> +            temp.parentNode.removeChild(temp);

This is going to have some side-effects, like firing unload events that appear to come from a tab, but don't actually map to any tab.

@@ +2811,5 @@
> +			
> +            temp.destroy();
> +            temp.parentNode.removeChild(temp);
> +  
> +            if (this.mTabBox) {

This check is unnecessary.

@@ +2812,5 @@
> +            temp.destroy();
> +            temp.parentNode.removeChild(temp);
> +  
> +            if (this.mTabBox) {
> +              tabPanel.removeChild(notificationbox);

notificationbox.remove();
Attachment #8768472 - Flags: feedback?(kmaglione+bmo) → feedback?(mconley)
(Assignee)

Comment 7

2 years ago
When 906076 lands, we can actually go a giant step further and strip the tab of the browser altogether and return it to its lazy-browser state.  Could be called "unlinkBrowser".  The resource recovery would be far more than simply "unloading".  Not suggesting that we wait on that though.
(Assignee)

Comment 8

2 years ago
Gijs, how does this bug depend on 906076?
Assignee: nobody → kernp25
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to Allasso Travesser from comment #8)
> Gijs, how does this bug depend on 906076?

I don't think there's much of a point replacing a tab's browser with a different one. Might as well just load about:blank into the existing browser. It also looks like the current code wouldn't fire the right unload events on the webpage. Feels like it'd make more sense to do this when we can actually lazily create browsers, which is what bug 906076 is about, AIUI.
Please leave it to kernp25 to decide when to formally take this bug. It's not even clear that we should do this at all (see comment 3).
Assignee: kernp25 → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Priority: -- → P5
(Reporter)

Comment 11

2 years ago
(In reply to Kris Maglione [:kmag] from comment #1)
> I think that if we were going to support this in WebExtensions, it would
> make more sense as a tab attribute, that could be queried and changed via
> tabs.update. I'm not sure that "unload" is the right name, though. But I
> can't immediately think of a better one, either.

Then it would like look like:
browser.tabs.update({pending: true});

I think this is ok!

And the unloadTab method maybe we add it to SessionStore.jsm?

Then we can also fix this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1128502
Comment on attachment 8768472 [details] [diff] [review]
bug-1284886-patch_1.diff

Redirecting to Gijs in the hopes of a quicker turn-around-time.
Attachment #8768472 - Flags: feedback?(mconley) → feedback?(gijskruitbosch+bugs)
Comment on attachment 8768472 [details] [diff] [review]
bug-1284886-patch_1.diff

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

Kris already outlined a number of issues with taking this patch. Besides the technical issues, I think we need a clearer reasoning about why we should do this / what the "point" is, and why we shouldn't wait for and prioritize the lazy browser framework first.

On a technical level, as I commented earlier the patch has some issues, like the lack of beforeunload events blocking this action, and the duplicate "temp" browser which doesn't seem to serve much of a purpose - I bet just removing and re-adding the browser that's already there is an easier way of achieving the same aim - which is something we should integrate with the lazy browser stuff so we just remove the browser and let the lazy browser stuff re-init a browser when we need one.

More generally, it feels like this is duplicating a general pattern of "how do I set up a browser" and "how do I tear down a browser" with the manual assignment of null to private browser properties like ._finder. Duplicating that stuff is going to be annoying because we'll need to make sure it all stays in sync and is ordering-correct for both remote and non-remote browsers, whenever we make tabbrowser changes. So really (a) it shouldn't be doing that duplicating (but reuse existing code instead), and (b) there should be automated tests for this functionality that come with the patch.
Attachment #8768472 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
(Reporter)

Comment 14

2 years ago
Chrome now has a discard feature: https://developer.chrome.com/extensions/tabs#method-discard
Can firefox have this too?
(In reply to kernp25 from comment #14)
> Chrome now has a discard feature:
> https://developer.chrome.com/extensions/tabs#method-discard
> Can firefox have this too?

That's a question for the webextensions team.
Component: Tabbed Browser → WebExtensions: Untriaged
Flags: needinfo?(kmaglione+bmo)
Product: Firefox → Toolkit
(In reply to kernp25 from comment #14)
> Chrome now has a discard feature:
> https://developer.chrome.com/extensions/tabs#method-discard
> Can firefox have this too?

Yes. We were planning on implementing something like that regardless.
Blocks: 1322485
Flags: needinfo?(kmaglione+bmo)

Updated

2 years ago
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → Tabbed Browser
Ever confirmed: true
Priority: P5 → --
Product: Toolkit → Firefox
Priority: -- → P5
Bug 1322485 is now on our list of webextensions+ bugs that we feel should be fixed by 57. It sounds like that means that this needs to be picked up again, as it is a blocker. There has been some discussion in this bug about abandoning the current proposed implementation and waiting for bug 906076 to land (in fact bug 906076 has been added as a blocker), but it's difficult to tell where the effort for that bug currently stands. There are patches and there has been discussion, but seemingly not for about 8 months.

I added a comment to bug 906076 to inquire about the likelihood of it landing in time to complete the rest of the work needed for  Bug 1322485 in 57, so we'll have to see what the response to that is. If that is unlikely to happen, does that mean we need to revisit the patch attached to this bug and see if it can land, or do we want to take a different approach from that?
(In reply to Bob Silverberg [:bsilverberg] from comment #18)
> Bug 1322485 is now on our list of webextensions+ bugs that we feel should be
> fixed by 57. It sounds like that means that this needs to be picked up
> again, as it is a blocker. There has been some discussion in this bug about
> abandoning the current proposed implementation and waiting for bug 906076 to
> land (in fact bug 906076 has been added as a blocker), but it's difficult to
> tell where the effort for that bug currently stands. There are patches and
> there has been discussion, but seemingly not for about 8 months.
> 
> I added a comment to bug 906076 to inquire about the likelihood of it
> landing in time to complete the rest of the work needed for  Bug 1322485 in
> 57, so we'll have to see what the response to that is. If that is unlikely
> to happen, does that mean we need to revisit the patch attached to this bug
> and see if it can land, or do we want to take a different approach from that?

Central parts of bug 906076 have already landed in dependent bugs. E.g. we can now call addTab to create a tab with a "lazy" browser.

However, there's no direct connection between bug 906076 and this one.
No longer depends on: 906076
Thanks Dão. Does this mean that this feature can now be implemented using the parts of bug 906076 that have already landed, which would address the concerns mentioned in comment #6 and comment #13? If so, do we know who is available to work on it? We need it to unblock bug 1322485.
Assignee: nobody → kmaglione+bmo
Priority: P5 → P1
(Assignee)

Comment 21

a year ago
Created attachment 8860702 [details] [diff] [review]
1284886_patch_lazify_browser_V1.diff

unloadTab method that reverts tab to a lazy browser state.

A few questions regarding SessionStore > ssi_onTabUnload:

Do we need to set DirtyWindows?

Do we need to update windows[window.__SSi].tabs[tab._tPos] with the state?

Or anything else in `restoreTab` we should do again?

Do we need to do an explicit update of TabStateCache? (It appears this should always be kept up to date, but just wondering if I'm missing something.)

Note that `didStartLoadSinceLastUserTyping` trap was added to tabbrowser.xml > _browserBindingProperties.  This is to prevent a bug which would occur if `getTabState` is called on a tab which was unloaded which had a userTypedValue.
Attachment #8860702 - Flags: feedback?(mdeboer)
Attachment #8860702 - Flags: feedback?(kmaglione+bmo)
Attachment #8860702 - Flags: feedback?(dao+bmo)
Comment on attachment 8860702 [details] [diff] [review]
1284886_patch_lazify_browser_V1.diff

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

As for naming, I'd be fine with mirroring Chrome's term 'discardTab', actually. I think it covers the load better than 'unload'.

As for the tabbrowser specific changes, I'd like to defer those to Dão for feedback.

::: browser/base/content/tabbrowser.xml
@@ +2034,5 @@
>          "addProgressListener", "removeProgressListener", "audioPlaybackStarted",
>          "audioPlaybackStopped", "adjustPriority", "pauseMedia", "stopMedia",
>          "blockMedia", "resumeMedia", "mute", "unmute", "blockedPopups", "lastURI",
>          "purgeSessionHistory", "stopScroll", "startScroll",
> +        "userTypedValue", "userTypedClear", "didStartLoadSinceLastUserTyping"

Where does this property come from?

::: browser/components/sessionstore/SessionStore.jsm
@@ +1886,5 @@
>      }
>      // The browser has been inserted now, so lazy data is no longer relevant.
>      delete aTab.__SS_lazyData;
>    },
> +  onTabUnload(aTab) {

Please don't forget to document this method.

@@ +1887,5 @@
>      // The browser has been inserted now, so lazy data is no longer relevant.
>      delete aTab.__SS_lazyData;
>    },
> +  onTabUnload(aTab) {
> +    let browser = aTab.linkedBrowser;

For posterity, I think you should check for `browser.isConnected` here too.

@@ +1890,5 @@
> +  onTabUnload(aTab) {
> +    let browser = aTab.linkedBrowser;
> +
> +    browser.removeEventListener("SwapDocShells", this);
> +    browser.removeEventListener("oop-browser-crashed", this);

Let's find a way to re-use `onTabRemove`
Attachment #8860702 - Flags: feedback?(mdeboer) → feedback+
(Assignee)

Comment 23

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #22)
> Comment on attachment 8860702 [details] [diff] [review]
> 1284886_patch_lazify_browser_V1.diff
> 
> Review of attachment 8860702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As for naming, I'd be fine with mirroring Chrome's term 'discardTab',
> actually. I think it covers the load better than 'unload'.

It sounds like you're getting rid of the tab :-/

> ::: browser/base/content/tabbrowser.xml
> @@ +2034,5 @@
> >          "addProgressListener", "removeProgressListener", "audioPlaybackStarted",
> >          "audioPlaybackStopped", "adjustPriority", "pauseMedia", "stopMedia",
> >          "blockMedia", "resumeMedia", "mute", "unmute", "blockedPopups", "lastURI",
> >          "purgeSessionHistory", "stopScroll", "startScroll",
> > +        "userTypedValue", "userTypedClear", "didStartLoadSinceLastUserTyping"
> 
> Where does this property come from?

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#887
(Assignee)

Comment 24

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #22)
> Comment on attachment 8860702 [details] [diff] [review]
> 1284886_patch_lazify_browser_V1.diff
> @@ +1890,5 @@
> > +  onTabUnload(aTab) {
> > +    let browser = aTab.linkedBrowser;
> > +
> > +    browser.removeEventListener("SwapDocShells", this);
> > +    browser.removeEventListener("oop-browser-crashed", this);
> 
> Let's find a way to re-use `onTabRemove`

The only thing onTabUnload and onTabRemove have in common is those three lines, otherwise they are very different.  It seems like it would be kind of messy to use conditionals to combine them both in onTabRemove.  They are also referring to two different events.  Or maybe I'm not understanding what you are asking?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 25

a year ago
Created attachment 8862854 [details] [diff] [review]
1284886_patch_lazify_browser_V2.diff
Attachment #8768472 - Attachment is obsolete: true
Attachment #8860702 - Attachment is obsolete: true
Attachment #8860702 - Flags: feedback?(kmaglione+bmo)
Attachment #8860702 - Flags: feedback?(dao+bmo)
Attachment #8862854 - Flags: feedback?(mdeboer)
Attachment #8862854 - Flags: feedback?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8862854 - Flags: feedback?(kmaglione+bmo)
(In reply to Kevin Jones from comment #23)
> It sounds like you're getting rid of the tab :-/

True. Let's keep it 'unload' then. We could also go for something more fancy: hibernate. (Source: bug 675539, which will be using whatever we end up shipping here.)

So how does 'hibernateTab' sound?

(In reply to Kevin Jones from comment #24)
> The only thing onTabUnload and onTabRemove have in common is those three
> lines, otherwise they are very different.  It seems like it would be kind of
> messy to use conditionals to combine them both in onTabRemove.  They are
> also referring to two different events.  Or maybe I'm not understanding what
> you are asking?

Yeah, I know, but what I mean is that I want to avoid having to modify multiple places, with the odd chance of forgetting one, when we add or remove or add a listener in the future. Generally, I'd like to avoid code duplication.
Flags: needinfo?(mdeboer)
Blocks: 675539
(Assignee)

Comment 27

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #26)
> (In reply to Kevin Jones from comment #23)
> > It sounds like you're getting rid of the tab :-/
> 
> True. Let's keep it 'unload' then. We could also go for something more
> fancy: hibernate. (Source: bug 675539, which will be using whatever we end
> up shipping here.)
> 
> So how does 'hibernateTab' sound?

I'd be okay with hibernateTab.  It is a bit more descriptive than unloadTab, and also makes the point that we're doing something different than the historical use of the term "unloading" as in addons.

> (In reply to Kevin Jones from comment #24)
> > The only thing onTabUnload and onTabRemove have in common is those three
> > lines, otherwise they are very different.  It seems like it would be kind of
> > messy to use conditionals to combine them both in onTabRemove.  They are
> > also referring to two different events.  Or maybe I'm not understanding what
> > you are asking?
> 
> Yeah, I know, but what I mean is that I want to avoid having to modify
> multiple places, with the odd chance of forgetting one, when we add or
> remove or add a listener in the future. Generally, I'd like to avoid code
> duplication.

That makes sense.  Just a suggestion, what if we keep both methods, and use a helper function for removing the listeners and any other common functionality?  I personally don't like the idea of amalgamating the two, it seems confusing to me especially if one is nested under the name of the other.

Just a suggestion though, I'm perfectly okay with using conditionals if that is what you prefer.

Comment 28

a year ago
Hi,
If I may interject, the reason this is a requested feature is that the #1 cause for Firefox crashing is having 100+ tabs and the Kernel OOM'ing it - which is even more common on 32-bit kernels.

To that end could care please be taken during the implementation of this issue such that checks are in place that memory mapped by Firefox gets re-released to the kernel? i.e. free() or related are called on the right amount of memory. The memory consumption after opening a tab, loading a site, and unloading/hivernating should be the same as in the beginning (+ small allowance for the visual representation of the tab hanging around in the tab bar, but, I guess, not more than tens of K per unloaded tab).

I think this is crucial; there is little cause to implement this API if this does not work.

Thank you!
(Assignee)

Comment 29

a year ago
I did some benchmarking:

500 tabs, startup, insert all browsers (insert into document only, no content was loaded), hibernate all browsers.
Avg. of 5 iterations:

                                 Main process        Sum of content processes
                                 resident MB         resident MB
startup:                         365  maxdev 11%       0  maxdev 0
after insert all browsers:       396  maxdev  2.2%   690  maxdev 1.8%
after hibernate all browsers:    394  maxdev  3.5%     0  maxdev 0

Seems like what I would expect.

What is disconcerting though was shutdown times.  The times here are an average of three iterations each:

Start-up only:                                                 2.13 sec  maxdev 4.2%
Start-up, instantiate all browsers (no content loaded):        4.70 sec  maxdev 3.2%
Start-up, instantiate all browsers, hibernate all browsers:    9.90 sec  maxdev 3.4%

It actually takes longer to shut down after hibernating all browsers than when shutting down with the browsers instantiated. :-/

I also had a crash on shut-down in one case.  I can't say for sure if it is related to this.  I will upload the crash report.

This was tested on a build before bug 1336763 landed and bug 1360940 appeared.

Tested on Nightly build 20170421030241, OS X 10.9.4.
(Assignee)

Comment 30

a year ago
Created attachment 8863294 [details]
crash_report_comment_29.txt

Comment 31

a year ago
Kevin thank you.

Of course I have no real say in how important this is to the Mozilla team but it is my feeling that the performance of this should be tested for each build, and regressions should be seen as build failures. I guess building up the testing infrastructure for this might be involved, but I believe this is important for lasting performance and good user experience. I suggest having a file in whatever repository holds the tests with definitions of acceptable performance for this and any other such tests, e.g. "500 tabs, 10 iterations -> 343 mb /0 mbbefore opening, 360 mb / 600 mb after opening and each restoring, and 360 mb / 0.1 mb after each hibernation pass". If this file is in a simple machine readable ascii format then people can keep track of it easily and people can vote in a transparent manner about what sort of perf is acceptable.

It comes to my attention that you are finding bugs by manual testing of the kind that should be tested with and caught by property testing but I don't know if there is a suitable implementation of QuickCheck for JS that would lend itself to this, and the topic is beside the point perhaps.
Comment on attachment 8862854 [details] [diff] [review]
1284886_patch_lazify_browser_V2.diff

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

I see you didn't implement the changes I proposed, which is fine I guess, but the changes are now so minimal that I don't think it's necessary for me to provide another round of feedback.
Attachment #8862854 - Flags: feedback?(mdeboer)
(Assignee)

Comment 33

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #32)
> Comment on attachment 8862854 [details] [diff] [review]
> 1284886_patch_lazify_browser_V2.diff
> 
> Review of attachment 8862854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see you didn't implement the changes I proposed, which is fine I guess,
> but the changes are now so minimal that I don't think it's necessary for me
> to provide another round of feedback.

I've been waiting to hear back from you regarding my second question in comment 27 before submitting another patch.  Maybe I forgot to n'info you.
(Assignee)

Comment 34

a year ago
ps, also, do you have any thoughts regarding comment 29, namely the extended shutdown time after tabs have been hibernated?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 35

a year ago
Hi Mike, I think this may keep slipping through the cracks due to my confused use of needinfo.  I am still waiting to hear your thoughts about the suggestion I made in the last part of comment 27, regarding adding a helper function for removing the listeners and common code for onTabHibernate/onTabRemove.  As I said, amalgamating the two would seem confusing to me especially with one nested under the name of the other.  But again, that was just a suggestion and I am happy to defer to combining them in onTabRemove if you prefer.
(In reply to Kevin Jones from comment #35)
> Hi Mike, I think this may keep slipping through the cracks due to my
> confused use of needinfo.  I am still waiting to hear your thoughts about
> the suggestion I made in the last part of comment 27, regarding adding a
> helper function for removing the listeners and common code for
> onTabHibernate/onTabRemove.  As I said, amalgamating the two would seem
> confusing to me especially with one nested under the name of the other.  But
> again, that was just a suggestion and I am happy to defer to combining them
> in onTabRemove if you prefer.

Ah, ok, sorry for the late reply! A helper function sounds good, Kevin.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 37

a year ago
Created attachment 8863748 [details] [diff] [review]
1284886_patch_lazify_browser_V3.diff

Changed method name to `hibernateTab`, added helper function for common code in onTabHibernate and onTabRemove in SessionStore.
Attachment #8862854 - Attachment is obsolete: true
Attachment #8862854 - Flags: feedback?(kmaglione+bmo)
Attachment #8862854 - Flags: feedback?(dao+bmo)
Attachment #8863748 - Flags: feedback?(mdeboer)
Attachment #8863748 - Flags: feedback?(dao+bmo)
Attachment #8863748 - Flags: feedback?(mdeboer) → feedback+
(Assignee)

Comment 38

a year ago
Hello Dao, been a while, any chance you might get to look at this?
Flags: needinfo?(dao+bmo)

Comment 39

a year ago
Would it make sense to assign this bug to Kevin, not Kris?
Comment on attachment 8863748 [details] [diff] [review]
1284886_patch_lazify_browser_V3.diff

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

::: browser/base/content/tabbrowser.xml
@@ +2251,5 @@
> +
> +            // Send event so SessionStore handler can remove listeners and
> +            // set up lazy restore data.  This must be sent before browser
> +            // is destroyed and removed from the document.
> +            var evt = new CustomEvent("TabHibernate", { bubbles: true, detail: {} });

This should probably be cancelable, and happen before listeners have been disconnected.

Also, are we sure it's OK to ignore beforeunload listeners here?

@@ +2263,5 @@
> +            aTab.removeAttribute("linkedpanel");
> +
> +            aTab._browserParams = { uriIsAboutBlank: true,
> +                                    remoteType: E10SUtils.NOT_REMOTE,
> +                                    usingPreloadedContent: false }

Nit: Missing semicolon.
Assignee: kmaglione+bmo → kevinhowjones

Comment 41

a year ago
Kevin, anything I can do to help on this?
(Assignee)

Comment 42

a year ago
(In reply to Andy McKay [:andym] from comment #41)
> Kevin, anything I can do to help on this?

Thanks Andy.  The situation I outlined in comment 29 and comment 30 is still a mystery to me.  I was hoping someone would spot something that would explain this on review, but it hasn't been addressed directly.  Perhaps addressing the comments from Kris's review (comment 40) might have an effect on this.  I haven't had time to implement these changes to see if it makes a difference but hopefully will have some time soon.  Maybe if you look at the comments mentioned you might have a sense if this might address the problem or not.  IAE, I don't think this will be able to land without the situation in comment 29 and comment 30 being addressed.
Flags: needinfo?(amckay)
(Assignee)

Comment 43

a year ago
Created attachment 8881352 [details] [diff] [review]
1284886_patch_lazify_browser_V4.diff

I've addressed your comments, Kris.  Please see if the handling of onbeforeunload looks something like what you had in mind.

I also discovered the bug causing the long shutdown times referred to in comment 29.  The TabHibernate handler in SS needs to reset the restoring state before the browser gets torn down.  Without this, when TabRemove handler gets called on a hibernated tab it attempts to reset the restoring state triggering browser insertion by accessing `messageManager`.
Attachment #8863294 - Attachment is obsolete: true
Attachment #8863748 - Attachment is obsolete: true
Attachment #8863748 - Flags: feedback?(dao+bmo)
Flags: needinfo?(dao+bmo)
Flags: needinfo?(amckay)
Attachment #8881352 - Flags: review?(kmaglione+bmo)
Comment on attachment 8881352 [details] [diff] [review]
1284886_patch_lazify_browser_V4.diff

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

This looks good, aside from a few nits. The only thing it's missing is tests. I think this is probably too fragile to land without them.

I'm also a bit wary of the beforeUnload handling, though. I think maybe we should just forbid hibernating tabs with beforeUnload handlers rather than calling permitUnload(). Aside from the potential perf issues when hibernating multiple tabs, I think users are likely to be confused if we switch to a tab and ask them if they want to close it, when all we're really trying to do is hibernate it (which should be more or less invisible to the user). And of course there's also the issue that the beforeunload prompt requires activating the tab, and we don't ever want to hibernate a tab that's active...

::: browser/base/content/tabbrowser.xml
@@ +2343,5 @@
> +          <![CDATA[
> +            "use strict";
> +
> +            // If aTab is selected, or is already unloaded, don't attempt to unload.
> +            if (aTab.selected || !aTab.linkedPanel || aTab.closing || this._windowIsClosing) {

We should probably throw an error if someone tries to call this for a selected tab.

@@ +2367,5 @@
> +
> +            // Send event so SessionStore handler can remove listeners and
> +            // set up lazy restore data.  This must be sent before browser
> +            // is destroyed and removed from the document.
> +            var evt = new CustomEvent("TabHibernate", { bubbles: true, detail: {} });

Should also have `cancelable: true`

::: browser/components/sessionstore/SessionStore.jsm
@@ +1878,5 @@
> +   * Also re-instate lazy data and basically revert tab to its lazy browser state.
> +   * @param aTab
> +   *        Tab reference
> +   */
> +  onTabHibernate(aTab) {

We should probably assert that the tab isn't active here.

@@ +1881,5 @@
> +   */
> +  onTabHibernate(aTab) {
> +    let browser = aTab.linkedBrowser;
> +    // Browser is already lazy so don't do anything.
> +    if (!browser.isConnected) { return; }

Three lines, please.

@@ +1885,5 @@
> +    if (!browser.isConnected) { return; }
> +
> +    this.cleanUpRemovedBrowser(aTab);
> +
> +    browser.setAttribute("pending", "true");

This shouldn't be necessary.

@@ +1888,5 @@
> +
> +    browser.setAttribute("pending", "true");
> +    aTab.setAttribute("pending", "true");
> +
> +    this._lastKnownFrameLoader.delete(browser.permanentKey);

I'm not sure we want or need this, but we probably should clear the "crashed" attribute and remove the browser from "crashedBrowsers".
Attachment #8881352 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 45

a year ago
(In reply to Kris Maglione [:kmag] from comment #44)
> I'm also a bit wary of the beforeUnload handling, though. I think maybe we
> should just forbid hibernating tabs with beforeUnload handlers rather than
> calling permitUnload(). Aside from the potential perf issues when
> hibernating multiple tabs, I think users are likely to be confused if we
> switch to a tab and ask them if they want to close it, when all we're really
> trying to do is hibernate it (which should be more or less invisible to the
> user). And of course there's also the issue that the beforeunload prompt
> requires activating the tab, and we don't ever want to hibernate a tab
> that's active...

Let's see if I am following you here.  So for example:

Add beforeunload listener on say tabContainer which will add eg `beforeunloadCalled` property to tab.
hibernateTab ignores tabs with this property.

I am wondering, if the user chose to cancel the unload, how the `beforeunloadCalled` property would get removed?

> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +1878,5 @@
> > +   * Also re-instate lazy data and basically revert tab to its lazy browser state.
> > +   * @param aTab
> > +   *        Tab reference
> > +   */
> > +  onTabHibernate(aTab) {
> 
> We should probably assert that the tab isn't active here.

So you are referring to the documentation?  Something like:

   * @param aTab
   *        Reference to a non-active tab

> @@ +1888,5 @@
> > +
> > +    browser.setAttribute("pending", "true");
> > +    aTab.setAttribute("pending", "true");
> > +
> > +    this._lastKnownFrameLoader.delete(browser.permanentKey);
> 
> I'm not sure we want or need this, but we probably should clear the
> "crashed" attribute and remove the browser from "crashedBrowsers".

So you are saying remove the

this._lastKnownFrameLoader.delete(browser.permanentKey);

line?
(Assignee)

Comment 46

a year ago
Also, please forgive for the bikeshed here, but for reasons I can't explain I really don't like "hibernateTab".

Would anyone object if we went with "suspendTab" instead?

If anyone objects I promise that is the last thing I'll say about it.
(In reply to Kevin Jones from comment #45)
> Let's see if I am following you here.  So for example:
>
> Add beforeunload listener on say tabContainer which will add eg
> `beforeunloadCalled` property to tab.
> hibernateTab ignores tabs with this property.

I don't care if beforeunload has been called, just whether there are any
beforeunload listeners that would trigger a confirmation dialog. See the
permitUnload method:

http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/toolkit/content/widgets/remote-browser.xml#290

> > We should probably assert that the tab isn't active here.
>
> So you are referring to the documentation?  Something like:
>
>    * @param aTab
>    *        Reference to a non-active tab

It should be documented, yes, but I mean that we should throw an error if a
caller tries to suspend an active tab, rather than just silently ignoring it.

> > I'm not sure we want or need this, but we probably should clear the
> > "crashed" attribute and remove the browser from "crashedBrowsers".
>
> So you are saying remove the
>
> this._lastKnownFrameLoader.delete(browser.permanentKey);
>
> line?

Yes.
(Assignee)

Comment 48

a year ago
(In reply to Kris Maglione [:kmag] from comment #47)
> > So you are referring to the documentation?  Something like:
> >
> >    * @param aTab
> >    *        Reference to a non-active tab
> 
> It should be documented, yes, but I mean that we should throw an error if a
> caller tries to suspend an active tab, rather than just silently ignoring it.

Okay, it confused me a bit because in a comment before that you suggested that we throw if aTab.selected, so I didn't know what you were saying beyond that.

Comment 49

a year ago
What's wrong with suspending/unloading the active tab? One might want to do that manually after identifying it to be consuming resources, so one can revisit it later.
(Assignee)

Comment 50

a year ago
(In reply to Kris Maglione [:kmag] from comment #47)
> I don't care if beforeunload has been called, just whether there are any
> beforeunload listeners that would trigger a confirmation dialog. See the
> permitUnload method:
> 
> http://searchfox.org/mozilla-central/rev/
> 17ebac68112bd635b458e831670c1e506ebc17ad/toolkit/content/widgets/remote-
> browser.xml#290

Please forgive me being thick here; I am not familiar with a way to tell whether listeners have been added.

Is this something we can determine by inspecting browser.frameLoader.tabParent.hasBeforeUnload?

And if false, it is safe to suspend the tab?
(Assignee)

Comment 51

a year ago
Kris, I'm n'info-ing you in case you didn't see comment 50 ^^^
Flags: needinfo?(kmaglione+bmo)
(In reply to Kevin Jones from comment #50)
> Please forgive me being thick here; I am not familiar with a way to tell
> whether listeners have been added.
> 
> Is this something we can determine by inspecting
> browser.frameLoader.tabParent.hasBeforeUnload?
> 
> And if false, it is safe to suspend the tab?

Yes, hasBeforeUnload tells you if there's a beforeunload listener, and if there isn't one, we can safely suspend the tab.

Ideally, even if there is a beforeunload listener, we should probably asynchronously trigger it, and abort suspending the tab if it tries to block it, without triggering a prompt. But I'm happy for that to be done in a follow-up.
Flags: needinfo?(kmaglione+bmo)

Comment 53

a year ago
Hello,
Please consider adding a beforeHibernate event to the API. If there are noth beforeUnload and beforeHibernate listeners present, that would semantically mean it is safe to hibernate the document, and only beforeHibernate should be called. beforeUnload should not be called if beforeHibernate is present during hibernating. This would be an opt-in for websites and it shouldn't block releasing the hibernation feature in the first place.
(In reply to cheater from comment #53)
> Please consider adding a beforeHibernate event to the API.

That's not an option. Web-visible events would need to be approved by a standards body, and I don't think such an event would give us any benefit over beforeunload.

Comment 55

a year ago
It would give you clear non-ambiguous indication of whether you can unload/hibernate or not when beforeUnload is present. It's not a blocker, though, so if it needs to be approved, it's fine for the approval to happen at its usual pace.
There's no need for a separate event for that. We can already dispatch beforeunload without triggering any UI.
(Assignee)

Comment 57

a year ago
Created attachment 8883130 [details] [diff] [review]
1284886_patch_lazify_browser_V6.diff

Kris, can you take a look at this and see if this meets your mind?

Tests still need to be written.
Attachment #8881352 - Attachment is obsolete: true
Attachment #8883130 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8883130 [details] [diff] [review]
1284886_patch_lazify_browser_V6.diff

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

::: browser/base/content/tabbrowser.xml
@@ +2349,5 @@
> +
> +            let browser = aTab.linkedBrowser;
> +
> +            if (!aTab.linkedPanel || aTab.closing || this._windowIsClosing ||
> +                browser.frameLoader.tabParent.hasBeforeUnload) {

This will work for remote browsers, but not local ones, which don't have a tabParent. So, for those, we'll either need a different way to determine if any beforeunload listeners exist, or just send the event, but skip the dialog (probably by adding an extra argument to nsIContentViewer.permitUnload)
Attachment #8883130 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Summary: add unloadTab method to tabbrowser.xml → add suspendTab method to tabbrowser.xml
(Assignee)

Comment 59

a year ago
(In reply to Kris Maglione [:kmag] from comment #58)
> Comment on attachment 8883130 [details] [diff] [review]
> 1284886_patch_lazify_browser_V6.diff
> 
> Review of attachment 8883130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +2349,5 @@
> > +
> > +            let browser = aTab.linkedBrowser;
> > +
> > +            if (!aTab.linkedPanel || aTab.closing || this._windowIsClosing ||
> > +                browser.frameLoader.tabParent.hasBeforeUnload) {
> 
> This will work for remote browsers, but not local ones, which don't have a
> tabParent. So, for those, we'll either need a different way to determine if
> any beforeunload listeners exist, or just send the event, but skip the
> dialog (probably by adding an extra argument to
> nsIContentViewer.permitUnload)

Thank you Kris.

Revealing my inexperience here, but I'm not seeing a path to implementing your suggestions.  I see that nsIContentViewer.permitUnload[1] does not take arguments.  I see permitUnloadInternal[2] which takes aShouldPrompt, but I don't find any scriptable code calling it.  I don't even know if these observations are relevant as I am not familiar with how C++ interfaces work.

I don't really know where to go from here.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIContentViewer.idl#56
[2] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIContentViewer.idl#68
Flags: needinfo?(kmaglione+bmo)

Comment 60

a year ago
I don't get why the "beforeUnload" event on a page. It doesn't add up why should a page receive information about the user exiting when, in reality, it should just be a "suspending" state where the tab returns to exactly as it was when the user comes back.
Instead, I recommend doing something else. Make a tab permission which a web page can request.
A use-case for such permission would be the usage of gmail. Many people want to keep gmail's tab active so that, on each e-mail received, the user gets a notification of sorts.

IMO, it is a permission that works like load images, allow popups, user the camera, etc... which web pages can request for. In the above use-case, gmail would request for permission for the tab not to suspend.

Any opinions or reasons not to do this?
Flags: needinfo?(kevinhowjones)
(In reply to brunoais from comment #60)
> I don't get why the "beforeUnload" event on a page. It doesn't add up why
> should a page receive information about the user exiting when, in reality,
> it should just be a "suspending" state where the tab returns to exactly as
> it was when the user comes back.

But it won't be "exactly the same" for many pages / apps, whose state isn't fully captured by the URL, cookies and form data (or whose form fields aren't available at the point where we try to fill them, maybe, my memory is fuzzy on how that works). We do the same thing for closing and then reopening a tab - we fire the beforeunload event.

To suspend the tab, we will be firing an unload event on the docshell, and the web specs say we should fire a beforeunload event before doing that (which obviously only makes sense if there are listeners for it). Breaking the spec here likely has lots of different unwanted consequences.

> Instead, I recommend doing something else. Make a tab permission which a web
> page can request.
> A use-case for such permission would be the usage of gmail. Many people want
> to keep gmail's tab active so that, on each e-mail received, the user gets a
> notification of sorts.
> 
> IMO, it is a permission that works like load images, allow popups, user the
> camera, etc... which web pages can request for. In the above use-case, gmail
> would request for permission for the tab not to suspend.
> 
> Any opinions or reasons not to do this?

It's not clear how you think this would work. If it's the same (tab won't suspend immediately, prompts the user) then I don't see the benefit over the beforeunload event. If the tab can somehow ask never to be suspended, that will likely annoy users - we should be able to suspend tabs that have state if the user asks us to.

Onbeforeunload is a somewhat-mitigated disaster, adding a similar thing is the wrong direction. If anything, we should eventually remove beforeunload altogether. What you're suggesting would be a new web API which takes a long time to standardize, never mind getting websites to actually use it, so it doesn't help at all for the long tail of sites, where beforeunload would Just Work because it's the same thing that websites already use to prompt users that they might lose data.

Comment 62

a year ago
(In reply to :Gijs from comment #61)
> (In reply to brunoais from comment #60)
> > I don't get why the "beforeUnload" event on a page. It doesn't add up why
> > should a page receive information about the user exiting when, in reality,
> > it should just be a "suspending" state where the tab returns to exactly as
> > it was when the user comes back.
> 
> But it won't be "exactly the same" for many pages / apps, whose state isn't
> fully captured by the URL, cookies and form data (or whose form fields
> aren't available at the point where we try to fill them, maybe, my memory is
> fuzzy on how that works). We do the same thing for closing and then
> reopening a tab - we fire the beforeunload event.
> 
> To suspend the tab, we will be firing an unload event on the docshell, and
> the web specs say we should fire a beforeunload event before doing that
> (which obviously only makes sense if there are listeners for it). Breaking
> the spec here likely has lots of different unwanted consequences.

If I understood right on how this works. When a tab is suspended, it is just like when I close firefox (without explicitly closing the tab) and then come back or when I go to a different page and then use the "back" button to return to the previous one.
All DOM state is kept and everything that is stored that is accessible from the 'window' object is kept in cache and even inside sessionstore.js file (when closing firefox). Is this wrong?

> 
> > Instead, I recommend doing something else. Make a tab permission which a web
> > page can request.
> > A use-case for such permission would be the usage of gmail. Many people want
> > to keep gmail's tab active so that, on each e-mail received, the user gets a
> > notification of sorts.
> > 
> > IMO, it is a permission that works like load images, allow popups, user the
> > camera, etc... which web pages can request for. In the above use-case, gmail
> > would request for permission for the tab not to suspend.
> > 
> > Any opinions or reasons not to do this?
> 
> It's not clear how you think this would work. If it's the same (tab won't
> suspend immediately, prompts the user) then I don't see the benefit over the
> beforeunload event. If the tab can somehow ask never to be suspended, that
> will likely annoy users - we should be able to suspend tabs that have state
> if the user asks us to.
A tab cannot request not to be suspended during suspension process, with this construct.
The idea would be that the web page would request for not being suspended every time it is loaded, preferably, after the `load` event but any time long-enough before the suspension will trigger the permissions request to the browser user.
The permission can also be stored to always allow and always disallow.

> Onbeforeunload is a somewhat-mitigated disaster, adding a similar thing is
> the wrong direction. If anything, we should eventually remove beforeunload
> altogether. What you're suggesting would be a new web API which takes a long
> time to standardize, never mind getting websites to actually use it, so it
> doesn't help at all for the long tail of sites, where beforeunload would
> Just Work because it's the same thing that websites already use to prompt
> users that they might lose data.
Imagine how useful would be for a browser user to receive let's say... generously; 50 consecutive warnings and jumping around from page to page telling that leaving the page can lead to data loss.
In a concrete situation:
There's tabs from A to Z, a generous total of 31 tabs.
The tabs from A to K will warn when the browser user leaves them, all the others won't.
The user is working on tab x.
A certain event happened and so an extension decides that all tabs from A to H and M to W should be suspended.

While trying to suspend A, the user, who was concentrated on tab X, immediately gets his attention towards tab A with a browser popup to attend so he can choose if he wants to leave the page or not.
As soon as he selects an option, the extension will try to suspend tab B
While trying to suspend B, the user, who was concentrated on tab X, immediately gets his attention towards tab B with a browser popup to attend so he can choose if he wants to leave the page or not.

The cycle continues. Do you get the problem here?

So what could be used to solve this? Not show the popup? If so, that means that all websites, can decide to standardize into preventing tab suspension and the user won't know that. I don't mean all websites will do such thing but they clearly can.
Not only the website will be in control, with such solution, the user (the one who should be in control) will be unable to dictate the outcome he wants.

As a solution for that, there's what I described above that you quoted.

Does that explain my POV well enough?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to brunoais from comment #62)
> All DOM state is kept and everything that is stored that is accessible from
> the 'window' object is kept in cache and even inside sessionstore.js file
> (when closing firefox). Is this wrong?

Yes, that's wrong. That is not how session restore works. We don't serialize the DOM to disk/cache/sessionstore.js. We might have certain information in the http cache, maybe, but that is independent of sessionstore and it *is* dependent on server headers and http cache settings / disk space, so it's not an assumption we can just rely on.

> Imagine how useful would be for a browser user to receive let's say...
> generously; 50 consecutive warnings and jumping around from page to page
> telling that leaving the page can lead to data loss.

That's not what comment #52 and later suggest. There won't be a warning. In any case, most pages don't have beforeunload handlers, so the whole discussion is basically a sideshow to getting the basic functionality implemented.

Please help keep this bug on track by not continuing this discussion now. Once the initial implementation lands and *if* you're unhappy with it then, file a followup bug with suggested remedies.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 64

a year ago
I was trying to call to a possible issue that I have no guaranty or study and provide an alternative (so I wouldn't just provide a possible problem). If you are aware, then alright.

Any way for a web extension to force a tab to suspend, regardless of its content's with this API?
(Assignee)

Updated

a year ago
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kevinhowjones)
(In reply to Kevin Jones from comment #59)
> > ::: browser/base/content/tabbrowser.xml
> > @@ +2349,5 @@
> > > +
> > > +            let browser = aTab.linkedBrowser;
> > > +
> > > +            if (!aTab.linkedPanel || aTab.closing || this._windowIsClosing ||
> > > +                browser.frameLoader.tabParent.hasBeforeUnload) {
> > 
> > This will work for remote browsers, but not local ones, which don't have a
> > tabParent. So, for those, we'll either need a different way to determine if
> > any beforeunload listeners exist, or just send the event, but skip the
> > dialog (probably by adding an extra argument to
> > nsIContentViewer.permitUnload)
> 
> Thank you Kris.
> 
> Revealing my inexperience here, but I'm not seeing a path to implementing
> your suggestions.  I see that nsIContentViewer.permitUnload[1] does not take
> arguments.  I see permitUnloadInternal[2] which takes aShouldPrompt, but I
> don't find any scriptable code calling it.  I don't even know if these
> observations are relevant as I am not familiar with how C++ interfaces work.
> 
> I don't really know where to go from here.

We can just decide not to support this feature without e10s. In fact I think e10s is supposed to be enabled for all users in 57.
(Assignee)

Comment 66

a year ago
(In reply to Dão Gottwald [::dao] from comment #65)
> We can just decide not to support this feature without e10s. In fact I think
> e10s is supposed to be enabled for all users in 57.

Sounds good (and simple).  Thanks Dao.
(Assignee)

Comment 67

a year ago
(In reply to Dão Gottwald [::dao] from comment #65)
> We can just decide not to support this feature without e10s. In fact I think
> e10s is supposed to be enabled for all users in 57.

Should we throw an assertion if e10s is not enabled?
Flags: needinfo?(dao+bmo)
I think just returning silently is fine.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 69

a year ago
Created attachment 8898544 [details] [diff] [review]
1284886_patch_suspendTab_V1.diff

Rebased, made suspendTab ignore non-remote tabs per Dao's suggestion, added tests.
Attachment #8883130 - Attachment is obsolete: true
Attachment #8898544 - Flags: review?(kmaglione+bmo)
Comment on attachment 8898544 [details] [diff] [review]
1284886_patch_suspendTab_V1.diff

>+            if (aTab.selected) {
>+              throw ("Active tab cannot be supended.");

I think we can just return silently in this case too.

>+            // Send event so SessionStore handler can remove listeners and
>+            // set up lazy restore data.  This must be sent before browser
>+            // is destroyed and removed from the document.
>+            let event = new CustomEvent("TabSuspend", { bubbles: true, cancelable: true });
>+            aTab.dispatchEvent(event);
>+            if (event.defaultPrevented) {
>+              return;
>+            }

Call SessionStore directly? The event just adds overhead.

>+  is(!!tab0.linkedPanel, false, "tab0 is suspended");

ok(!tab0.linkedPanel, "tab0 is suspended");
(Assignee)

Comment 71

a year ago
(In reply to Dão Gottwald [::dao] from comment #70)
> Comment on attachment 8898544 [details] [diff] [review]
> 1284886_patch_suspendTab_V1.diff
> 
> >+            if (aTab.selected) {
> >+              throw ("Active tab cannot be supended.");
> 
> I think we can just return silently in this case too.

Kris suggested specifically in comment 44 to throw in this case :-/

> >+            // Send event so SessionStore handler can remove listeners and
> >+            // set up lazy restore data.  This must be sent before browser
> >+            // is destroyed and removed from the document.
> >+            let event = new CustomEvent("TabSuspend", { bubbles: true, cancelable: true });
> >+            aTab.dispatchEvent(event);
> >+            if (event.defaultPrevented) {
> >+              return;
> >+            }
> 
> Call SessionStore directly? The event just adds overhead.

I anticipate we'll want an event when bug 1322485 gets implemented.  I think addons are going to want to know when this occurs.

Also, using events seems to be the scheme with stuff like this in SessionStore: TabClose, TabOpen, TabSelect etc.
(Assignee)

Comment 72

a year ago
(In reply to Kevin Jones from comment #71)
> > Call SessionStore directly? The event just adds overhead.
> 
> I anticipate we'll want an event when bug 1322485 gets implemented.  I think
> addons are going to want to know when this occurs.
> 
> Also, using events seems to be the scheme with stuff like this in
> SessionStore: TabClose, TabOpen, TabSelect etc.

Also, would we want to expose another method from SessionStore?
(In reply to Kevin Jones from comment #71)
> > >+            if (aTab.selected) {
> > >+              throw ("Active tab cannot be supended.");
> > 
> > I think we can just return silently in this case too.
> 
> Kris suggested specifically in comment 44 to throw in this case :-/

I see no reason to throw an exception here.

> > Call SessionStore directly? The event just adds overhead.
> 
> I anticipate we'll want an event when bug 1322485 gets implemented.  I think
> addons are going to want to know when this occurs.

Let's not anticipate but implement this if and when we need it.

> Also, using events seems to be the scheme with stuff like this in
> SessionStore: TabClose, TabOpen, TabSelect etc.

The code dispatching these events doesn't depend on sessionstore. suspendTab can't work without it.
(Assignee)

Comment 74

a year ago
Created attachment 8900267 [details] [diff] [review]
1284886_patch_suspendTab_V2.diff

Updated per comments.
Attachment #8898544 - Attachment is obsolete: true
Attachment #8898544 - Flags: review?(kmaglione+bmo)
Attachment #8900267 - Flags: review?(dao+bmo)
Comment on attachment 8900267 [details] [diff] [review]
1284886_patch_suspendTab_V2.diff

>+      <method name="suspendTab">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+            "use strict";
>+
>+            let browser = aTab.linkedBrowser;

So to be more consistent with the expected webextension API, how about we call this discardBrowser? The parameter should be the browser then, and you can get the tab via getTabForBrowser.

>+  ok(!!tab0.linkedPanel, "tab0 is no longer suspended");

nit: remove !!
(Assignee)

Comment 77

a year ago
Created attachment 8900335 [details] [diff] [review]
1284886_patch_suspendTab_V3.diff

Updated per comments.
Attachment #8900267 - Attachment is obsolete: true
Attachment #8900267 - Flags: review?(dao+bmo)
Attachment #8900335 - Flags: review?(dao+bmo)
(Assignee)

Comment 78

a year ago
Comment on attachment 8900335 [details] [diff] [review]
1284886_patch_suspendTab_V3.diff

I didn't read you comment carefully enough...
Attachment #8900335 - Attachment is obsolete: true
Attachment #8900335 - Flags: review?(dao+bmo)
(Assignee)

Comment 79

a year ago
Created attachment 8900341 [details] [diff] [review]
1284886_patch_suspendTab_V4.diff

This should be better.
Attachment #8900341 - Flags: review?(dao+bmo)
Comment on attachment 8900341 [details] [diff] [review]
1284886_patch_suspendTab_V4.diff

>+            let tab = this.getTabForBrowser(aBrowser);
>+
>+            if (tab.selected || !aBrowser.isConnected || tab.closing || this._windowIsClosing ||
>+                !aBrowser.isRemoteBrowser || aBrowser.frameLoader.tabParent.hasBeforeUnload) {
>+              return;
>+            }

Should also null-check tab.

>+            // Remove listeners and set up lazy restore data in SessionStore.  This
>+            // must be called before aBrowser is destroyed and removed from the document.
>+            SessionStore.cleanUpBeforeDiscardingBrowser(tab);

Can you rename this? SessionStore does more than cleaning up here.

>+            this.mPanelContainer.removeChild(notificationbox);
>+            notificationbox.removeAttribute("id");

What's the point of removing the id attribute?

>+            tab._browserParams = { uriIsAboutBlank: true,
>+                                    remoteType: E10SUtils.NOT_REMOTE,

Why uriIsAboutBlank: true and why remoteType: E10SUtils.NOT_REMOTE?

>+  ok(!!tab0.linkedPanel, "active tab is not able to be suspended");

Remove !!
Attachment #8900341 - Flags: review?(dao+bmo)
(Assignee)

Comment 81

a year ago
(In reply to Dão Gottwald [::dao] from comment #80)
> Comment on attachment 8900341 [details] [diff] [review]
> 1284886_patch_suspendTab_V4.diff
> >+  ok(!!tab0.linkedPanel, "active tab is not able to be suspended");
> 
> Remove !!

Are you saying don't attempt to try to suspend the active tab and remove the test altogether?  Or just remove the "ok" assertion there because the next "ok" assertion after attempting to remove a closed tab will verify that anyway?
Remove the two exclamation marks...
(Assignee)

Comment 83

a year ago
(In reply to Dão Gottwald [::dao] from comment #82)
> Remove the two exclamation marks...

ooooooh.... I thought you were being emphatic (seriously) :-)
(Assignee)

Comment 84

a year ago
(In reply to Dão Gottwald [::dao] from comment #80)
> Comment on attachment 8900341 [details] [diff] [review]
> >+            // Remove listeners and set up lazy restore data in SessionStore.  This
> >+            // must be called before aBrowser is destroyed and removed from the document.
> >+            SessionStore.cleanUpBeforeDiscardingBrowser(tab);
> 
> Can you rename this? SessionStore does more than cleaning up here.

Can you suggest a name?

> >+            this.mPanelContainer.removeChild(notificationbox);
> >+            notificationbox.removeAttribute("id");
> 
> What's the point of removing the id attribute?

I don't know, I thought it may be an artifact from old removeTab code, but for life of me I can't find it anywhere.  So it can probably just go away.

> >+            tab._browserParams = { uriIsAboutBlank: true,
> >+                                    remoteType: E10SUtils.NOT_REMOTE,
> 
> Why uriIsAboutBlank: true and why remoteType: E10SUtils.NOT_REMOTE?

I am trying to return the tab to the state it would be in if it were a new tab on startup, and this is how a lazy tab starts out.  I believe this ends up being the result after code runs in addTab and E10SUtils.getRemoteTypeForURI() for a lazy tab.  I don't think having a remoteType of anything other than NOT_REMOTE would be meaningful for a tab with an uninstantiated browser, would it?

But maybe it makes no difference and those properties don't need to be reset.  I think once the browser gets instantiated they change anyway.
(Assignee)

Comment 85

a year ago
Created attachment 8900842 [details] [diff] [review]
1284886_discardBrowser_V1.diff

Updated per comments.  I took a stab at the name.
Attachment #8900341 - Attachment is obsolete: true
Attachment #8900842 - Flags: review?(dao+bmo)
Comment on attachment 8900842 [details] [diff] [review]
1284886_discardBrowser_V1.diff

>+            if (!tab || tab.selected || !aBrowser.isConnected || tab.closing ||
>+                this._windowIsClosing || !aBrowser.isRemoteBrowser ||
>+                aBrowser.frameLoader.tabParent.hasBeforeUnload) {

I'd prefer having each condition on its own line.

>+            tab._browserParams = { uriIsAboutBlank: true,
>+                                    remoteType: E10SUtils.NOT_REMOTE,
>+                                    usingPreloadedContent: false };

nit: too much indentation

How about using aBrowser.currentURI.spec == "about:blank" and aBrowser.remoteType?
Attachment #8900842 - Flags: review?(dao+bmo)
(Assignee)

Comment 87

a year ago
Created attachment 8901526 [details] [diff] [review]
1284886_discardBrowser_V2.diff

Updated per comments.
Attachment #8900842 - Attachment is obsolete: true
Attachment #8901526 - Flags: review?(dao+bmo)
(Assignee)

Comment 89

a year ago
Created attachment 8901527 [details] [diff] [review]
1284886_discardBrowser_V3.diff

Fixed eslint error.
Attachment #8901526 - Attachment is obsolete: true
Attachment #8901526 - Flags: review?(dao+bmo)
Attachment #8901527 - Flags: review?(dao+bmo)
(Assignee)

Comment 90

a year ago
(In reply to Kevin Jones from comment #88)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9d6cd5178833a5e867f22e95f20f8831a03fd8af

There doesn't seem to be any errors related to this patch (eslint error was fixed on the last patch).  Does everything look okay to you Dao?
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #90)
> (In reply to Kevin Jones from comment #88)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=9d6cd5178833a5e867f22e95f20f8831a03fd8af
> 
> There doesn't seem to be any errors related to this patch (eslint error was
> fixed on the last patch).  Does everything look okay to you Dao?

Looks good.
Flags: needinfo?(dao+bmo)
Comment on attachment 8901527 [details] [diff] [review]
1284886_discardBrowser_V3.diff

>+            tab._browserParams = { uriIsAboutBlank: aBrowser.currentURI.spec == "about:blank",
>+                                   remoteType: aBrowser.remoteType,
>+                                   usingPreloadedContent: false };

This should happen earlier where aBrowser.currentURI and aBrowser.remoteType are guaranteed to still exist.
Attachment #8901527 - Flags: review?(dao+bmo)
(Assignee)

Comment 93

a year ago
(In reply to Dão Gottwald [::dao] from comment #92)
> Comment on attachment 8901527 [details] [diff] [review]
> 1284886_discardBrowser_V3.diff
> 
> >+            tab._browserParams = { uriIsAboutBlank: aBrowser.currentURI.spec == "about:blank",
> >+                                   remoteType: aBrowser.remoteType,
> >+                                   usingPreloadedContent: false };
> 
> This should happen earlier where aBrowser.currentURI and aBrowser.remoteType
> are guaranteed to still exist.

I don't follow your thinking here.  We are returning the tab/browser to the lazy state.  Don't we want _browserParams to reflect that, and not the state before it was made lazy?
Flags: needinfo?(dao+bmo)
No, the lazy state itself is when this stuff doesn't matter at all. _browserParams is used when inserting the browser.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 95

a year ago
Created attachment 8902270 [details] [diff] [review]
1284886_discardBrowser_V4.diff

Updated per comments.
Attachment #8901527 - Attachment is obsolete: true
Attachment #8902270 - Flags: review?(dao+bmo)
Comment on attachment 8902270 [details] [diff] [review]
1284886_discardBrowser_V4.diff

>+            // Remove listeners and set up lazy restore data in SessionStore.  Also
>+            // set browser parameters for when browser is restored.  This must be
>+            // done before aBrowser is destroyed and removed from the document.
>+            SessionStore.resetBrowserToLazyState(tab);
>+
>+            tab._browserParams = { uriIsAboutBlank: aBrowser.currentURI.spec == "about:blank",
>+                                   remoteType: aBrowser.remoteType,
>+                                   usingPreloadedContent: false };

Please set tab._browserParams even before calling SessionStore.resetBrowserToLazyState.

Thanks!
Attachment #8902270 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 98

a year ago
Created attachment 8903976 [details] [diff] [review]
1284886_discardBrowser_V5.diff

Final update.
Attachment #8902270 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Summary: add suspendTab method to tabbrowser.xml → Add discardBrowser method to tabbrowser.xml
Needs a rebased patch.
Flags: needinfo?(kevinhowjones)
Keywords: checkin-needed
(Assignee)

Comment 100

a year ago
Created attachment 8904078 [details] [diff] [review]
1284886_discardBrowser_V6.diff

Rebased.
Attachment #8903976 - Attachment is obsolete: true
Flags: needinfo?(kevinhowjones)
Attachment #8904078 - Flags: checkin?(ryanvm)
(Assignee)

Comment 101

a year ago
Comment on attachment 8904078 [details] [diff] [review]
1284886_discardBrowser_V6.diff

Something went wrong with this patch, trying again.
Attachment #8904078 - Attachment is obsolete: true
Attachment #8904078 - Flags: checkin?(ryanvm)
(Assignee)

Comment 102

a year ago
Comment on attachment 8904078 [details] [diff] [review]
1284886_discardBrowser_V6.diff

Apparently there is nothing wrong with this patch, I was confounded by the interdiff results.
Attachment #8904078 - Attachment is obsolete: false
Attachment #8904078 - Flags: checkin?(ryanvm)
Attachment #8904078 - Flags: checkin?(ryanvm)
Keywords: checkin-needed

Comment 103

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e59ce2ce01
Add discardBrowser method to tabbrowser.xml, which returns tab to its lazy state. r=dao, r=mikedeboer
Keywords: checkin-needed
Backed out for failing own test on Windows 7 debug without e10s:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72d94b046a5a259de815ef2a79fce959cd24cfbb

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128341774&repo=mozilla-inbound

16:52:53     INFO -  191 INFO TEST-START | browser/components/sessionstore/test/browser_1284886_suspend_tab.js
16:52:53     INFO -  GECKO(1472) | ++DOCSHELL 2961E400 == 12 [pid = 1472] [id = {efd25e48-e6da-4219-af77-5e075aa16d77}]
16:52:53     INFO -  GECKO(1472) | ++DOMWINDOW == 29 (2961EC00) [pid = 1472] [serial = 29] [outer = 00000000]
16:52:53     INFO -  GECKO(1472) | ++DOMWINDOW == 30 (29621000) [pid = 1472] [serial = 30] [outer = 2961EC00]
16:52:53     INFO -  GECKO(1472) | ++DOMWINDOW == 31 (29626800) [pid = 1472] [serial = 31] [outer = 2961EC00]
16:52:53     INFO -  GECKO(1472) | [1472] WARNING: attempt to modify an immutable nsStandardURL: file z:/build/build/src/netwerk/base/nsStandardURL.cpp, line 1827
16:52:53     INFO -  TEST-INFO | started process screenshot
16:52:53     INFO -  TEST-INFO | screenshot: exit 0
16:52:53     INFO -  Buffered messages logged at 16:52:53
16:52:53     INFO -  192 INFO Entering test bound test
16:52:53     INFO -  Buffered messages finished
16:52:53    ERROR -  193 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_1284886_suspend_tab.js | tab0 is suspended -
16:52:53     INFO -  Stack trace:
16:52:53     INFO -  chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_1284886_suspend_tab.js:test:10
16:52:53     INFO -  GECKO(1472) | [1472] WARNING: We've already scheduled a task for background list flush.: file z:/build/build/src/parser/html/nsHtml5TreeOpExecutor.cpp, line 288
16:52:53     INFO -  GECKO(1472) | [1472] WARNING: We've already scheduled a task for background list flush.: file z:/build/build/src/parser/html/nsHtml5TreeOpExecutor.cpp, line 288
Flags: needinfo?(kevinhowjones)
I thought we'd stopped running non-e10s tests?

In any case, let's just disable this test in non-e10s modes, that method intentionally only handles remote browsers.
(Assignee)

Comment 106

a year ago
Created attachment 8904373 [details] [diff] [review]
1284886_discardBrowser_V7.diff

Resubmitting patch with e10s disabled for tests.
Attachment #8904078 - Attachment is obsolete: true
Flags: needinfo?(kevinhowjones)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 107

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/662fb967d092
Implement discardBrowser method. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/662fb967d092
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

11 months ago
Depends on: 1405442
Duplicate of this bug: 1342280
(Assignee)

Updated

9 months ago
Depends on: 1415918

Updated

8 months ago
Blocks: 1427928

Updated

7 months ago
Depends on: 1427822

Updated

4 months ago
Depends on: 1451799

Updated

3 months ago
Blocks: 1462813

Updated

3 months ago
No longer blocks: 1427928
You need to log in before you can comment on or make changes to this bug.