Bug 906076 (lazytabs)

Virtual tabs - lazily create linkedBrowser and other dependent elements for tabbrowser tabs to improve startup performance

NEW
Assigned to

Status

()

Firefox
Tabbed Browser
P2
enhancement
4 years ago
12 days ago

People

(Reporter: Kevin Jones, Assigned: Kevin Jones, Mentored)

Tracking

(Depends on: 2 bugs, Blocks: 7 bugs, {footprint, meta, perf})

Trunk
footprint, meta, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 45 obsolete attachments)

16.05 KB, image/png
Details
(Assignee)

Description

4 years ago
This is just a suggestion/question for future development:

Nowadays, power users using 100+ tabs, sometimes 500 to 600, are becoming common.  Even without loading a pages in the tabs, it still takes long startup times and large memory footprint.

What about having "virtual" tabs?  That is, until a tab is loaded, there is simply a minimal placeholder in its place.  The actual tab object is still not built.  The sessionstore tab data is still on disk.  The tab only gets built when it is selected, where it calls the data from disk JIT.

It would seem that the cost of doing a quick read from disk every now and then would be trivial compared to the annoyance of building all 500 tabs at once.  In most cases it would not even be noticeable.  Also, it is often that, even though a user may want to keep 500 tabs available, they may only load 100 of them, or less in a given session.  So the total memory footprint (and possibility of leakage) would be less, even at the end of the session.  And of course, at the beginning of the session it would be a LOT less.

Of course, there could be a tab count threshold before this feature is implemented. (though I think that reading from disk is so trivial these days, you could just implement it all the time)

What do you think?  Has anyone thought of this already?

I would love to do this as an addon, but I cannot see a way to get around this in a non-invasive way without sessionstore, and unfortunately I am not able to extend sessionstore - at least that I am aware of.

Am I wrong about not being extend sessionstore?  If anyone can suggest a way to get around that (without bypassing Firefox sessionstore functionality) I would get on it right away.

Of course, it would be optimal if this functionality were built into Firefox.

It seems like this would have quite a large impact in user experience for power users, and even most users these days.

Am I just being naive?
Allasso, you might try asking some of these questions on irc on #developers . There are also some existing add-ons you might take a look at, as they need work to stay compatible with current versions of Firefox. https://addons.mozilla.org/en-US/firefox/addon/bartab/  and https://addons.mozilla.org/en-US/firefox/addon/load-tabs-progressively/.   I have also linked some related bugs to this one so you can take a look at previous discussions or suggestions that are relevant!
Severity: normal → enhancement
Component: Untriaged → Tabbed Browser
See Also: → bug 441213, bug 257453
(Assignee)

Comment 2

4 years ago
Thank you.  I believe these addons only manage loading/unloading of tabs content (the page within the tabbrowser).  My own addon All Tabs Helper does that as well.

What I am talking about here is different, I am talking about **creating the tab objects** on-demand.
Severity: enhancement → normal
Component: Tabbed Browser → Untriaged
Component: Untriaged → Tabbed Browser
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

4 years ago
Severity: normal → enhancement
Flags: needinfo?(dteller)
(Assignee)

Comment 3

3 years ago
I have been developing an addon (called Virtual Tabs) which has endeavored to emulate what is described here.  A rough overview is this:

"Virtual" tabs are normal tab elements that are created and placed in a scrollbox alongside of tabbox.  tabbox is then collapsed.  The actual tabbrowser tabs are never visible to the user.  The idea is that the user sees these lightweight virtual tabs, and there are actually no tabbrowser tabs present until one is needed.  So "restore on demand" actually means, "create the tab and the linked browser and restore it" on demand.  When a tab is "unloaded", the tab is actually removed, and won't be created again until there is a demand to load the tab.  A virtual tab only carries enough data to create the tab.  The user can determine how much restoration data they want to save.  For the needs of many users, this can be quite small.  To the user, the interface and experience is the same as before.

I am far into development with this, and the results are very promising (exciting actually).  A 1000 tab session which normally takes about a minute to load, loads virtually instantly with Virtual Tabs.  With timed unload features and user practices which are common today among mega-tab users, the memory footprint can be kept very small even throughout the sesssion.

Perhaps this will provide a gateway to reconsidering our concept of the tabbrowser tab, and restructuring the tab to an object that only has what is needed when it is needed.
That's great to hear. Besides loading time, is there any visible difference for the user?
Flags: needinfo?(dteller)
(Assignee)

Comment 5

3 years ago
No,(In reply to David Rajchenbach Teller [:Yoric] (away until August 20th - use "needinfo") from comment #4)
> That's great to hear. Besides loading time, is there any visible difference
> for the user?

No, the interface looks and works the same.
In that case, this sounds like something we might want as a set of patches rather than an add-on. How difficult would it be for you to rewrite your code as a sequence of patches that could be reviewed and landed incrementally?
(Assignee)

Comment 7

3 years ago
I would be interested in trying that.
I believe that it would be great. Needinfo?-ing Tim, as I'm not a peer on that part of Firefox. 

I don't think you have worked on patches yet, so you'll probably want to head to https://developer.mozilla.org/en-US/docs/introduction to get started.
Flags: needinfo?(ttaubert)
(Assignee)

Comment 9

3 years ago
Thank you.  I was wondering how to get started
Allasso, do you have any code to look at somewhere? On GitHub maybe? From comment #3 I can't exactly tell how your add-on works and whether that is something we can do in Firefox without breaking too many existing add-ons. This might be worth looking into before you start spending a lot of time on writing patches :)
Flags: needinfo?(ttaubert) → needinfo?(allassopraise)
(Assignee)

Comment 11

3 years ago
The extension I started writing probably would break a lot of addons.  I started writing it mainly as a demonstration of what could be done and what the results would be (extremely fast restoring of high volume sessions for one), and also something for my own use.  Since it is an addon, I had to create these additional "virtual" tabs which the user would see in place of the tabbrowser tabs.

But the concept that I have been promoting is to make the tabs very lightweight upon creation, and only add all of the heavyweight stuff when it is needed (when the user is ready to load the tab), for example the linked browser.  Really, just the stuff to make it look and feel like a tab, and the state data for the tab for when the user is ready to load it. (even the state data I believe can be significantly reduced on a lot of tabs, but that is probably for another bug)  It never occurred to me that I could write a patch to do this, and that really is the proper solution.  Doing it this way, I think the impact on extensions would be very minimal, if at all, unless an extension is accessing the linked browser on unloaded tabs for some reason.

So the patch I am working on is to do just that.  It deals with the issue directly instead of making a workaround for it, like the addon does.  The results I have seen in my preliminary development has been very encouraging.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 12

3 years ago
May someone please assign this to me?
To be honest, I'm not comfortable with assigning this bug before we know whether there is any approach we can take that wouldn't affect all add-ons out there and that we would actually want to implement. I still have no good oversight over the plan and haven't seen any code yet :/

We also need to keep in mind that we can't pay a too high price in terms of breaking existing add-ons or rewriting tons of code to support that because that majority of users just has not enough tabs to care about whether five of them would be virtual or not. If changes are intrusive and help power-users then in the end it might be better off living inside an add-on and being an optional feature.

But this is all just assumptions and guessing without understanding the approach.
Dão is a great person to take a look at the approach as well.
(Assignee)

Comment 15

3 years ago
@:ttaubert:

I understand the concerns, and have considered them myself as well.  And not seeing any code I understand your feelings.  I thought I had given a fairly good oversight, but then I'm the one who understands it and maybe I haven't described the approach very well, or maybe it was too conceptual and you are looking for something more nuts and bolts.  I know seeing some code would give a clearer view of it.  I have been working hard on this daily since it was first suggested, and have wanted to offer something fairly solid before just throwing some code out for examination.  If there is not enough code to actually demonstrate the benefit, then it would be difficult to justify even considering.  Also the approach has been evolutionary and there has been a lot of experimentation.  This combined with the fact that I am inherently slow is why nothing has been revealed yet, however I feel I am in a place to upload some preliminary stuff in the next day or three.  I need to do some regression and break down what I have so I can show it in stages as has been suggested.

Concerning an addon, as far as I can see it would be very difficult to create one that didn't pretty much break every tab addon there is, and probably other stuff as well.  This would be partly because of the inability to extend SessionStore, and partly because of the complexity of the tabbed browsing system itself.  But maybe there is an approach to it with an addon that I have not seen yet.

I have nothing to press, and will understand if this never lands.  I just deal with a lot of power users and am very aware of the frustrations they come against.  I have been very encouraged by what I have seen this patch is able to accomplish, so admittedly I am a bit excited about it.

Okay, enough talk, let's see some code... :-)
FWIW, being assigned to a bug is mostly a formality. You should be able to just post what you have and ask for feedback on it, and go from there :)
(Assignee)

Comment 17

3 years ago
Created attachment 8482033 [details] [diff] [review]
Bug_906076_Patch_1_tabbrowser.xml_modified_addTab.diff

tabbrowser.xml -> addTab patch - addTab restructured and broken in two to allow for lightweight tabs functionality.  Implementing this patch with no other changes should have no effect on Firefox functionality.  Only when the arguments[1].dontLinkBrowser parameter is set will this behave differently.
(Assignee)

Comment 18

3 years ago
Created attachment 8482035 [details] [diff] [review]
Bug_906076_Patch_2_SessionStore.jsm_tabbrowser.xml_restoreWindow_and_various_functions.diff

SessionStore.jsm - windowRestore modified to restore a window to virtual tabs if var "lightweight" is set.  This is a stub for future pref which will allow Firefox to function in traditional mode if wanted.  restoreLightweightTabs method added.  Various other code which looks for linkedBrowser has been adjusted to deal with accordingly.

tabbrowser.xml - begin/endRemoveTab have been adjusted to accomodate lightweight tabs (no linkedBrowser)
(Assignee)

Comment 19

3 years ago
I uploaded two progressive patches for examination and feedback. The first patch can be implemented without affecting the app, however, the second patch will break some functionality until subsequent patches are implemented.  One notable example is that the user is not yet able to click a tab to link the browser and load it.

I have also uploaded a video demonstration of the user experience of this concept.  The code in the video is more advanced than these two patches, and a lot of the normal functionality has been restored.  However, in the case of the performance of tab restoring on startup and mid-session restores which is demonstrated in the video, these two patches will provide that.

https://www.youtube.com/watch?v=VoAsjaW8L_M

I did some benchmarking (not the rough timing that is done in the video) based on when the tabs begin to restore to when the user interface becomes functional, and generally speaking, Firefox with Virtual Tabs restore about 10 times faster for larger sessions.  The tabs also take up about 1/10th the memory footprint.
(Assignee)

Comment 20

3 years ago
I also want to mention that windowRestore in the current patches (when "lightweight" is true) do not re-use tabs.  Future patches should fix that.
(Assignee)

Comment 21

3 years ago
Created attachment 8482050 [details]
Bug_906076_Patch_1_tabbrowser.xml_addTab_linBrowserToTab.txt

This is patch code used for the addTab mod; it is addTab and linkBrowserToTab intact.  Because the code that was separated out was so interspersed in the original, I felt it would be easier to interpret the changes this way than through the diff which looks like swiss cheese.
(Assignee)

Comment 22

3 years ago
Created attachment 8486460 [details]
stub to mark previous patches as obsolete

Marking all previous patches as obsolete.  I am upgrading code with new strategies which makes changes much less invasive and (hopefully) more friendly to addons.  Also involves much fewer code changes.  New patches will replace these.
Attachment #8482033 - Attachment is obsolete: true
Attachment #8482035 - Attachment is obsolete: true
Attachment #8482050 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8486460 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8489493 [details] [diff] [review]
part_1_structure_for_lightweight_tab.diff

Initial structure for the lightweight tab. Expands addTab into two function, addTab and linkBrowserToTab.   This gives the option to not link the browser until later, in which case a browser "stub" is substituted providing minimal "dummy" properties primarily for the purpose of satisfying other methods that expect them.

An additional parameter was added to addTab to tell it not to link the browser. If parameter is unset, addTab works as always.  Thus this part of the patch should have no effect on Firefox.
(Assignee)

Comment 24

3 years ago
Created attachment 8489511 [details] [diff] [review]
part_2_session_restore_functionality_rec.diff

Upgrades restoreWindow and restoreTabs in SessionStore.jsm to have option to to create lightweight tabs upon restore.  Also upgrades tabbrowser.xml to further accomodate lw tabs, such as reducing full tabs back to lightweight.  These upgrades include code to accomodate storage of sessions which are closed with lightweight tabs.

Note that with lightweight=true, sessions will restore to lightweight tabs even if they were closed with full-weight tabs.
(Assignee)

Comment 25

3 years ago
Created attachment 8489513 [details] [diff] [review]
part_3_tabbrowser_mods.diff

additional functionality in tabbrowser.xml to link browser and load lightweight tabs
(Assignee)

Comment 26

3 years ago
Created attachment 8489515 [details] [diff] [review]
part_4_webbrowser_mod.diff

very minor, fixes dev tools UI problem
(Assignee)

Comment 27

3 years ago
Created attachment 8489518 [details] [diff] [review]
part_5_unload_tab_UI_and_history_menu_image_fix.diff

minor changes, UI to give user Unload Tab capability, thus reducing full tabs to lightweight, also corrects problem with images sometimes not appearing in closed tabs history when lw tabs are closed.
(Assignee)

Comment 28

3 years ago
I have uploaded 5 attachments for the patch I wish to propose. The strategies have been changed a bit from the initial patch that was uploaded.  I would appreciate any feedback.

This covers the lightweight tab functionality to a large degree and basic tabs bar operations such as moving and closing tabs seems to be intact, as well as storage of sessions which are closed with lightweight tabs. Although I am sure there are some holes to be filled: one known case is the lack of proper functionality when dragging lightweight tabs back and forth between windows.

Note that I have been calling these "lightweight" tabs rather than "virtual" tabs.  Lightweight seems to better describe the concept, which originally had separate "virtual" tabs which would create tabbrowser tabs on demand that functioned hidden in the background.  Now we are actually creating tabbrowser tabs but just not linking the browser until needed, which though is a little more expensive that the virtual tab concept, is a much less invasive approach. Perhaps another term that would be descriptive is "lazy" tabs.

Also note that the performance gains are still the same with the upgraded strategy as those posted in comment 19, roughly 10 times the memory footprint and 10 times faster restore speed.
(Assignee)

Comment 29

3 years ago
Created attachment 8489757 [details] [diff] [review]
part_6_tab_state_preserve_upgrade.diff

Same as part 5, only removes the use of extData for saving state of lw tabs on session close. This is no longer necessary since we are explicitly setting linkedBrowser.__SS_data on the unloaded tab to the last state before unloading.
Attachment #8489518 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
(In reply to Allasso Travesser from comment #28)
> ...roughly 10 times the memory
> footprint...

Of course I meant 1/10 the memory.
(Assignee)

Comment 31

3 years ago
Created attachment 8489781 [details] [diff] [review]
part_7_fixes_coding_error_in_part_6.diff

Fixes serious coding error in part 6
Attachment #8489757 - Attachment is obsolete: true

Comment 32

3 years ago
Would you be willing to put the code in a repo on Bitbucket or Github?  I think that would be a better mechanism for making changes, as well as for people browsing and perhaps commenting on issues.  Bugzilla isn't really suited for this sort of thing.
(Assignee)

Comment 33

3 years ago
(In reply to Brendan from comment #32)
> Would you be willing to put the code in a repo on Bitbucket or Github?  I
> think that would be a better mechanism for making changes, as well as for
> people browsing and perhaps commenting on issues.  Bugzilla isn't really
> suited for this sort of thing.

Sure.  Please tell me if I am not doing this right...
https://Allasso@bitbucket.org/Allasso/allassorepos.git
(Assignee)

Comment 34

3 years ago
(In reply to Allasso Travesser from comment #33)
> Sure.  Please tell me if I am not doing this right...
> https://Allasso@bitbucket.org/Allasso/allassorepos.git

Please use this instead:
https://Allasso@bitbucket.org/Allasso/bugzilla_906076.git
Allasso, I just took a look at your patches and remembered that I had a similar idea quite a while ago that instead of a "fake" <browser> element set a JS Proxy to .linkedBrowser. Whenever that was accessed in any way it would quickly create a <browser> element once, forward the action once, and then replace .linkedBrowser with the now created <browser> element.

This is somewhat better than your approach because it means we wouldn't break any existing code. I am sure that there are tons of add-ons out there which rely on .linkedBrowser being a valid element as soon as the <tab> was created. We also have lots of tests that are based on the same assumption.

While that sounds all fine the actual work is then to get rid of all the code that immediately accesses .linkedBrowser for newly created background tabs, or only for pending tabs that are waiting to be restored. Finding the call sites is quite easy using a JS debugger or simply printing stack traces but actually fixing those sites is much harder.

With the "lazy browser" approach there could still be add-ons out there accessing the browser immediately after the tab was created and thus render our efforts void, but we could probably print a warning to the console and it wouldn't affect users that use the browser with default settings.
(Assignee)

Comment 36

3 years ago
so sorry, please use this instead:
https://Allasso@bitbucket.org/Allasso/bugzilla_906076_repo.git
(Assignee)

Comment 37

3 years ago
@ttaubert:
I see.  My original approach started off in this vein, but I was able to eliminate so many code changes with the fake browser.  I am not familiar with JS Proxy yet, but I don't suppose there is a way to combine both approaches is there, so as to satisfy FF code with prescribed "fake" returns while using the proxy for addons?

So the warning in the console would be to encourage addon devs to adjust their code to preserve the lazy tabs?
(Assignee)

Comment 38

3 years ago
Created attachment 8513751 [details] [diff] [review]
Bug_906076_PH3_lazy_p01_tabbrowser.xml_1_addTab_mod.diff

addTab extension to support lazy browser etc. attachment
Attachment #8513751 - Flags: feedback?
(Assignee)

Comment 39

3 years ago
Created attachment 8513754 [details] [diff] [review]
Bug_906076_PH3_lazy_p02_tabbrowser.xml_2.diff

mainly code to deal with linkedBrowser/lightweightTab
(Assignee)

Updated

3 years ago
Attachment #8513754 - Flags: feedback?
(Assignee)

Comment 40

3 years ago
Created attachment 8513755 [details] [diff] [review]
Bug_906076_PH3_lazy_p03_SessionStore.jsm_1.diff

Session restore to lightweight tabs functionality
Attachment #8513755 - Flags: feedback?
(Assignee)

Comment 41

3 years ago
Created attachment 8513756 [details] [diff] [review]
Bug_906076_PH3_lazy_p04_SessionStore.jsm_2.diff

mainly code to deal with linkedBrowser/lightweightTab
Attachment #8513756 - Flags: feedback?
(Assignee)

Comment 42

3 years ago
Created attachment 8513758 [details] [diff] [review]
Bug_906076_PH3_lazy_p05_TabState.jsm.diff

code to deal with linkedBrowser/lightweightTab
Comment on attachment 8513754 [details] [diff] [review]
Bug_906076_PH3_lazy_p02_tabbrowser.xml_2.diff

Asking for feedback without specifying a person doesn't actually do anything :)
Attachment #8513754 - Flags: feedback? → feedback?(ttaubert)
Attachment #8513751 - Flags: feedback? → feedback?(ttaubert)
Attachment #8513755 - Flags: feedback? → feedback?(ttaubert)
Attachment #8513756 - Flags: feedback? → feedback?(ttaubert)
Attachment #8513758 - Flags: feedback?(ttaubert)
(Assignee)

Comment 44

3 years ago
Created attachment 8513760 [details] [diff] [review]
Bug_906076_PH3_lazy_p06_RecentlyClosedTabsAndWindowsMenuUtils.jsm.diff

code to deal with linkedBrowser/lightweightTab
Attachment #8513760 - Flags: feedback?(ttaubert)
(Assignee)

Comment 45

3 years ago
Created attachment 8513763 [details] [diff] [review]
Bug_906076_PH3_lazy_p07_browser.diff

code to deal with linkedBrowser/lightweightTab
Attachment #8513763 - Flags: feedback?(ttaubert)
(Assignee)

Comment 46

3 years ago
Created attachment 8513766 [details] [diff] [review]
Bug_906076_PH3_lazy_p08_devtools.diff

code to deal with linkedBrowser/lightweightTab
Attachment #8513766 - Flags: feedback?(ttaubert)
(Assignee)

Comment 47

3 years ago
Created attachment 8513767 [details] [diff] [review]
Bug_906076_PH3_lazy_p09_tabview.diff

code to deal with linkedBrowser/lightweightTab
Attachment #8513767 - Flags: feedback?(ttaubert)
(Assignee)

Comment 48

3 years ago
Created attachment 8513769 [details] [diff] [review]
Bug_906076_PH3_lazy_p10_in-content__main.js.diff

code to deal with linkedBrowser/lightweightTab
Attachment #8513769 - Flags: feedback?(ttaubert)
(Assignee)

Comment 49

3 years ago
Created attachment 8513770 [details] [diff] [review]
Bug_906076_PH3_lazy_p11_webbrowser.js.diff

code to deal with linkedBrowser/lightweightTab
Attachment #8513770 - Flags: feedback?(ttaubert)
(Assignee)

Comment 50

3 years ago
I've uploaded a series of new patches in which tabs use a lazy getter for linkedBrowser, as Tim suggested. The escalation of a "lightweight" tab will be forced if linkedBrowser is accessed.  I have attempted however to link the browser (and structure several other elements as well) from Firefox code explicitly by calling linkBrowserToTab in an effort to provide ability to observe a distinction from tabs escalated by addons, possibly inadvertently.

I've added a check for the pref browser.sessionstore.lightweightTabs to allow toggling FFdefault/lightweight tabs for testing.  The pref is user set right now; unless it is explicitly set by the user, the patch will default to use lightweight tabs.
(Assignee)

Comment 51

3 years ago
Created attachment 8516787 [details] [diff] [review]
Bug_906076_PH3_lazy_p03_SessionStore.jsm_1.1.diff

Corrects error on line 2668, changed tab.urlToLoad to tab.faceURI.
Attachment #8513755 - Attachment is obsolete: true
Attachment #8513755 - Flags: feedback?(ttaubert)
Attachment #8516787 - Flags: feedback?(ttaubert)
(Assignee)

Comment 52

3 years ago
Created attachment 8518610 [details] [diff] [review]
Bug_906076_PH3_lazy_p05_TabState.2.jsm.diff

updated TabState.jsm patch with additional code changes
Attachment #8513758 - Attachment is obsolete: true
Attachment #8513758 - Flags: feedback?(ttaubert)
Attachment #8518610 - Flags: feedback?(ttaubert)
Sorry for making you wait so long on feedback. I think the current approach is not quite what we would want to land, unfortunately. We can't just change everything about the tabbrowser without breaking lots of existing code, and even worse lots of existing add-ons.

We should sit down and come up with a solid plan first before diving head first into it and writing patches. This feature is important to people with lots of tabs but it is not so much for the average person. We can't possibly justify breaking most tab-related add-ons by doing this for a low (but vocal) percentage of our user base.

With JavaScript and existing tools we can be a little more clever, e.g. use a Proxy to create a linkedBrowser lazily on access. Where at first all code still tries to access the browser we would then go and iteratively get rid of those code paths, or simply rewrite them. We could have a test that ensures nothing is accessing the .linkedBrowser too early. The tab.lightweightTab flag is a little too brittle for my taste and just requires to fix *lots* of existing code - introducing even more code paths with branches.
Attachment #8513751 - Flags: feedback?(ttaubert) → feedback-
Attachment #8513754 - Flags: feedback?(ttaubert)
Attachment #8513756 - Flags: feedback?(ttaubert)
Attachment #8513760 - Flags: feedback?(ttaubert)
Attachment #8513763 - Flags: feedback?(ttaubert)
Attachment #8513766 - Flags: feedback?(ttaubert)
Attachment #8513767 - Flags: feedback?(ttaubert)
Attachment #8513769 - Flags: feedback?(ttaubert)
Attachment #8513770 - Flags: feedback?(ttaubert)
Attachment #8516787 - Flags: feedback?(ttaubert)
Attachment #8518610 - Flags: feedback?(ttaubert)
Blocks: 1111169
Keywords: perf
(Assignee)

Updated

3 years ago
Summary: Virtual tabs → Virtual tabs - lazily create linkedBrowser and other dependent elements for tabbrowser tabs to improve startup performance
(Assignee)

Updated

3 years ago
Attachment #8489493 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8489511 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8489513 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8489515 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8489781 - Attachment is obsolete: true
Mentor: dteller@mozilla.com
(Assignee)

Comment 54

3 years ago
After an off-bug discussion with David, it seems it would be good to start small.  A good first step approach seems to be to only focus on start-up performance.  We could write a patch which would modify addTab to link the browser lazily.  We would only try to prevent tab escalation during start-up.  Once session restore has been completed, let come what may, and any access to linkedBrowser will immediately escalate the tab.  Most likely all tabs would be escalated in short order, and thus would not really gain any memory reduction advantange, but we would *greatly* reduce the amount of code changes from my original patch, while still getting the benefit of quick start-up for large numbers of tabs.

The tabbrowser modifications would mostly be in addTab, and could be very similar to the addTab mods in my last patch.  This would include the lazy browser linking.  There would also be a few modifications needed in sessionstore.js, such as restoreTab which if unmodified would immediately escalate the tabs upon restoring.  At this phase there shouldn't (AFAICT) be any changes needed to browser.js, tabview, etc.
I suspect you may need to export a field `isLinkedBrowserInstantiated` (working name) in xul:tab that evaluates to `yes` once `get linkedBrowser()` has been called at least once, `false` until then. This should simplify testing (e.g. "at the end of session restore, do we have more than one tab in which the `linkedBrowser` has been instantiated) and may also be useful for various optimizations.
I don't think we want to have a |isLinkedBrowserInstantiated| property that we use for optimizations. I can only see that this would make fragile code that is hard to read and is cluttered with lots of conditions.

The only way to make this work would be to turn |linkedBrowser| into a lazy getter or else we would break far too many add-ons. We can't break lots of add-ons only to optimize for a not so common use-case with lots of tabs.

The lazy getter could be implemented (as I said before) using a Proxy - and we'd have to do it for |gBrowser.browsers| too, that would void the optimization completely if we leave it as is. From there on we could iteratively get rid of places that access .linkedBrowser too soon after a tab was created, using |new Error().stack| to find out what those are. I've done that before and have patches lying around.

I talked to Gijs in Portland about the feasibility and whether it's worthwhile to do this. We thought it actually depends on how many places we'd have to fix and how hard it would be to fix them. The first step would thous be to have these Proxy patches only for testing, without actually landing them. Then take a look at what we have to fix and try estimating how hard that would be and determine whether it's worth doing that at all.

In reality, all of our efforts might still be in vain if there are just enough add-ons accessing .linkedBrowser "too early". To reiterate: I do not think it is worth breaking add-ons to make Firefox faster for power users - a group I would say I belong to as well with numerous tabs being restored on startup.
I forgot about that the biggest chunk of work with adding a lazy getter is that you will first run into sessionstore using the .linkedBrowser. sessionstore restores a tab by creating it and immediately injecting data into the <browser>. You would have to teach sessionstore to first just create the tab and do the rest upon actual restoration of the tab. This however would possibly break switch-to-tab and may break reloading a pending tab (not sure about that). There might be other regressions I didn't think of yet.
The main use case I have for `isLazyBrowserInstantiated` (or whatever it's called) is making sure that Session Restore doesn't immediately force evaluation of the `linkedBrowser` lazy getters accidentally 15 seconds after startup. There may be a solution that doesn't require it, but regardless, that's certainly not something that would break add-ons.

Anyway, let's start with making things lazy, then see what breaks.
(Assignee)

Comment 59

3 years ago
In thinking about strategies to deal with unwanted tab escalation during startup, I have considered these possibilities:

1) A flag on the tab indicating whether it is in its escalated state or not, as David suggested (isLazyBrowserInstantiated), and which I used on my patches (lightweightTab).  To me it is the simplest way of dealing with the matter.  The flag is set to one state upon tab creation, then the other state once the tab has been escalated.  Then methods which would tend to escalate the tab when not wanted (ie, during session restore for now) would test for the flag.  Tim, perhaps you could explain to me the fragile-ness of using this test, for I am having difficulty understanding what you are seeing.  In my mind, it is just a very common coding technique on the most basic level - set a property, test for it - yay or nay.  It is true, it would involve an additional test for methods which wish to use `linkedBrowser` and wish to be conscious of tab escalation.  The safety catch however, is if the test is not made, ie, by unsuspecting addons, then the tab will just escalate, and everything should run as expected.  In other words, the lazy browser is written in such a way that the default action is always to revert to normal Firefox, and thus (theoretically) not break any addons which are not optimized to use lazy browser.  This is just my unseasoned opinion on the matter, and Tim I respect your decision here.  BTW, the way I have written the `linkedBrowser` getter, I am setting a property `_linkedBrowser` to the "linked browser", and returning that property when `linkedBrowser` is accessed.  So really we would only need to test for `_linkedBrowser` which is already present/not present.

2) A globally accessible flag 1, eg, gBrowser.sessionRestoring: This flag would be set during session restore.  The `linkedBrowser` getter would not escalate the tab while the flag is set.  Some prescribed time After the session is restored, the flag is unset and accessing `linkedBrowser` will always escalate the tab.

3) A globally accessible flag 2: This flag would be set during session restore and unset after restore is finished.  Then methods in sessionstore.js which would tend to escalate tabs would test for this flag and not escalate when it is set.  This would have the advantage over #2 in that addons which may access `linkedBrowser` on startup would not be broken.

I see method 2 and 3 as only applying to the first phase of the lazy browser patch, which is only dealing with startup performance.  At some point, if we wish to evolve to a system which endeavors to remain lightweight until the tab is actually needed (in order to reap the lower memory consumption benefit), at least with a default configuration, then methods 2 and 3 would be inadequate.  It seems that method 1 would cover all cases, and as I said before, the default action would be to not break the addon.  But perhaps there is another way of accomplishing that goal without using the flag on the tab.
(Assignee)

Comment 60

3 years ago
"escalate tab" == "link browser to tab"
(Assignee)

Comment 61

3 years ago
Created attachment 8538206 [details] [diff] [review]
bug_906076_PH4_lazy_browser_addTab.diff

tabbrowser addTab patch with lazy browser.  Browser is linked when .linkedBrowser property is accessed, by `linkBrowserToTab` method.

By itself, this patch will probably appear to have no effect (tabs will behave as always), since .linkedBrowser will immediately be accessed by restoreTab during restore.  It is accessed immediately also when switching tabs, or user adding new tab, (at least) by onTabAdd handler in sessionstore.
Attachment #8513751 - Attachment is obsolete: true
Attachment #8513754 - Attachment is obsolete: true
Attachment #8513756 - Attachment is obsolete: true
Attachment #8513760 - Attachment is obsolete: true
Attachment #8513763 - Attachment is obsolete: true
Attachment #8513766 - Attachment is obsolete: true
Attachment #8513767 - Attachment is obsolete: true
Attachment #8513769 - Attachment is obsolete: true
Attachment #8513770 - Attachment is obsolete: true
Attachment #8516787 - Attachment is obsolete: true
Attachment #8518610 - Attachment is obsolete: true
Attachment #8538206 - Flags: feedback?(ttaubert)
Attachment #8538206 - Flags: feedback?(dteller)
Comment on attachment 8538206 [details] [diff] [review]
bug_906076_PH4_lazy_browser_addTab.diff

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

For the moment, I mostly have small remarks.
What's your next step?

::: browser/base/content/tabbrowser.xml
@@ +1640,5 @@
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="linkBrowserToTab">

Nit: it's a private method, so please either prefix it with "_" or make it a closure.

@@ +1646,5 @@
> +        <parameter name="aURI"/>
> +        <parameter name="aParams"/>
> +        <body>
> +          <![CDATA[
> +            let event = document.createEvent("Events");

Mmmh... You are adding a new notification, right?
You should document it.

@@ +1649,5 @@
> +          <![CDATA[
> +            let event = document.createEvent("Events");
> +            event.initEvent("TabLinkBrowser", true, false);
> +            aTab.dispatchEvent(event);
> +              

Nit: Not important at this stage, but don't forget to get rid of lines that contain only whitespaces.

@@ +1652,5 @@
> +            aTab.dispatchEvent(event);
> +              
> +            const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +            var aAllowThirdPartyFixup = aParams.mAllowThirdPartyFixup;

Nit: These are not arguments, so don't prefix them with `a`. Also, the options are not really members, so don't prefix them with `m`.

Oh, and could you take the opportunity to convert `let` to `var` in this method?

@@ +1661,5 @@
> +            var aPostData             = aParams.mPostData;
> +
> +            var uriIsAboutBlank = !aURI || aURI == "about:blank";
> +            
> +            // invalidate cache

I think that we have invalidated the cache also when creating the xul:browser. I'm not sure exactly what this does, but you need to triple-check that it's done at the right time – and to document this better than it was initially documented, because this is a side-effect caused by lazy instantiation.

@@ +1664,5 @@
> +            
> +            // invalidate cache
> +            this._browsers = null;
> +
> +            let b;

Could you take the opportunity to call this `browser` instead of `b`?

@@ +1686,5 @@
> +            let notificationbox = this.getNotificationBox(b);
> +            var uniqueId = this._generateUniquePanelID();
> +            notificationbox.id = uniqueId;
> +            aTab.linkedPanel = uniqueId;
> +            aTab._linkedBrowser = b;

You should document `_linkedBrowser` – or, if you manage, hide it entirely.

@@ +1721,5 @@
> +            // then let's just continue loading the page normally.
> +            if (!usingPreloadedContent && !uriIsAboutBlank) {
> +              // pretend the user typed this so it'll be available till
> +              // the document successfully loads
> +              if (aURI && gInitialPages.indexOf(aURI) == -1)

Could you take the opportunity to add curly braces around the body of this `if`?

@@ +1742,5 @@
> +            }
> +
> +            // We start our browsers out as inactive, and then maintain
> +            // activeness in the tab switcher.
> +            b.docShellIsActive = false;

It's not clear to me what `docShellIsActive` does, exactly, have you checked that this is the right place to do it?

@@ +1817,5 @@
>              // If this new tab is owned by another, assert that relationship
>              if (aOwner)
>                t.owner = aOwner;
>  
> +            t.faceURI = aURI;

What's `faceURI`?

@@ +1825,5 @@
> +            var _this = this;
> +            Object.defineProperty(t,"linkedBrowser",{
> +              get: function linkedBrowser_get() {
> +                if (!t._linkedBrowser) {
> +                  console.log("linking browser to tab");

I assume that this is for debugging purposes?

@@ +1827,5 @@
> +              get: function linkedBrowser_get() {
> +                if (!t._linkedBrowser) {
> +                  console.log("linking browser to tab");
> +
> +                  var prms = {

Nit: `let` is better than `var`. Also, `options` has become the de facto standard name for such arguments, rather than `prms`.

@@ +1836,5 @@
> +                    mCharset              : aCharset,
> +                    mPostData             : aPostData,
> +                  };
> +                  
> +                  _this.linkBrowserToTab(t,aURI,prms);

Nit: instead of using `_this`, turn `linkedBrowser_get` into a `=>` and use `this`.

@@ -1779,5 @@
>              // entirely done, so that things are in a consistent state
>              // even if the event listener opens or closes tabs.
>              var evt = document.createEvent("Events");
>              evt.initEvent("TabOpen", true, false);
>              t.dispatchEvent(evt);

I hope that we have good testing for this in our code, as the hypotheses here are not valid anymore.
Attachment #8538206 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 63

3 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #62)
> What's your next step?

To look for all places where browser is linked prematurely during startup, and determine a way of dealing with those so the browser does not get linked.

> Mmmh... You are adding a new notification, right?
> You should document it.

by document, you mean document in comments in the code?  Maybe a notification here is premature.

> @@ +1664,5 @@
> > +            
> > +            // invalidate cache
> > +            this._browsers = null;
> > +
> > +            let b;
> 
> Could you take the opportunity to call this `browser` instead of `b`?

Okay.  I called it `b` because that is the term used in the original code.

> @@ +1721,5 @@
> > +            // then let's just continue loading the page normally.
> > +            if (!usingPreloadedContent && !uriIsAboutBlank) {
> > +              // pretend the user typed this so it'll be available till
> > +              // the document successfully loads
> > +              if (aURI && gInitialPages.indexOf(aURI) == -1)
> 
> Could you take the opportunity to add curly braces around the body of this
> `if`?

again, that is how it is in the original

> @@ +1742,5 @@
> > +            }
> > +
> > +            // We start our browsers out as inactive, and then maintain
> > +            // activeness in the tab switcher.
> > +            b.docShellIsActive = false;
> 
> It's not clear to me what `docShellIsActive` does, exactly, have you checked
> that this is the right place to do it?

I believe it is, it follows the same chronology as the original, with the exception that it will always be after code that is in addTab.

> @@ +1817,5 @@
> >              // If this new tab is owned by another, assert that relationship
> >              if (aOwner)
> >                t.owner = aOwner;
> >  
> > +            t.faceURI = aURI;
> 
> What's `faceURI`?

that is a carryover from my previous patch.  It is a way for the tab to hold the uri while in its unlinked state, so the tabs uri can be accessed without using linkedBrowser.uri.spec.  Probably not necessary yet at this stage.  Don't ask me where I came up with the name...

> @@ +1825,5 @@
> > +            var _this = this;
> > +            Object.defineProperty(t,"linkedBrowser",{
> > +              get: function linkedBrowser_get() {
> > +                if (!t._linkedBrowser) {
> > +                  console.log("linking browser to tab");
> 
> I assume that this is for debugging purposes?

this is from a suggestion from Tim (comment #35 - though re-reading it now I see he actually suggested a warning), to let addon devs know that they have linked the browser.  Again, probably premature, but incidentally does help in debugging anyway.

> @@ -1779,5 @@
> >              // entirely done, so that things are in a consistent state
> >              // even if the event listener opens or closes tabs.
> >              var evt = document.createEvent("Events");
> >              evt.initEvent("TabOpen", true, false);
> >              t.dispatchEvent(evt);
> 
> I hope that we have good testing for this in our code, as the hypotheses
> here are not valid anymore.

do you mean, because the tab may not now be in the expected state when TabOpen is fired?
(In reply to Allasso Travesser from comment #63)
> (In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January
> 10th - use "needinfo") from comment #62)
> > What's your next step?
> 
> To look for all places where browser is linked prematurely during startup,
> and determine a way of dealing with those so the browser does not get linked.

Sounds good.

> > Mmmh... You are adding a new notification, right?
> > You should document it.
> 
> by document, you mean document in comments in the code?  Maybe a
> notification here is premature.

I think so.


> > Could you take the opportunity to call this `browser` instead of `b`?
> 
> Okay.  I called it `b` because that is the term used in the original code.

Here and below: indeed, but let's take the opportunity to leave the code better in a better state than you found it.

> > @@ +1742,5 @@
> > > +            }
> > > +
> > > +            // We start our browsers out as inactive, and then maintain
> > > +            // activeness in the tab switcher.
> > > +            b.docShellIsActive = false;
> > 
> > It's not clear to me what `docShellIsActive` does, exactly, have you checked
> > that this is the right place to do it?
> 
> I believe it is, it follows the same chronology as the original, with the
> exception that it will always be after code that is in addTab.

Well, in the original, this is done synchronously, while you do it asynchronously.
Now, looking at the implementation of `docShellIsActive`, I have the impression that it starts as `false` anyway, but I may be wrong.

> > @@ +1825,5 @@
> > > +            var _this = this;
> > > +            Object.defineProperty(t,"linkedBrowser",{
> > > +              get: function linkedBrowser_get() {
> > > +                if (!t._linkedBrowser) {
> > > +                  console.log("linking browser to tab");
> > 
> > I assume that this is for debugging purposes?
> 
> this is from a suggestion from Tim (comment #35 - though re-reading it now I
> see he actually suggested a warning), to let addon devs know that they have
> linked the browser.  Again, probably premature, but incidentally does help
> in debugging anyway.

Before this becomes something useful for addon devs, this will need a bit more work, but as a debugging feature, that's ok.

> > I hope that we have good testing for this in our code, as the hypotheses
> > here are not valid anymore.
> 
> do you mean, because the tab may not now be in the expected state when
> TabOpen is fired?

Indeed.
(Assignee)

Comment 65

3 years ago
Created attachment 8550898 [details] [diff] [review]
tabbrowser.xml - lazy browser, proxy `browsers`

tabbrowser.xml:

revised lazy browser code.  Uses proxy and instantiates browser when a property of linkedBrowser is accessed.  This allows trapping of linkedBrowser.property to allow substitution of properties where allowable. eg, browser.currentURI can be subsituted with URI object from tab data instead of unnecessarily instantiating.

Also uses proxy for get_browsers to avoid instantiation of browsers on all tabs once get_browsers is accessed
Attachment #8538206 - Attachment is obsolete: true
Attachment #8538206 - Flags: feedback?(ttaubert)
(Assignee)

Comment 66

3 years ago
Created attachment 8550899 [details] [diff] [review]
SessionStore.jsm optimized to support lazy browser
(Assignee)

Comment 67

3 years ago
Created attachment 8550900 [details] [diff] [review]
NetworkPrioritizer.jsm : has code to avoid unnecessary browser instantiation on start-up
(Assignee)

Comment 68

3 years ago
Created attachment 8550901 [details] [diff] [review]
tabbox.xml : has code to force instantiation of browser if tab is selected

code to force instantiation of browser upon tab selection was placed here because in some instances tab selection is done by setting selectedIndex on the tabbox.
(Assignee)

Comment 69

3 years ago
Created attachment 8550902 [details] [diff] [review]
TabState.jsm: code for saving state of tabs with un-instantiated browser
(Assignee)

Comment 70

3 years ago
attachment 8550902 [details] [diff] [review]
has code to properly save tab state for tabs that have not had the browser instantiated
(Assignee)

Comment 71

3 years ago
@David - regarding comment 62:

invalidating the cache by setting `this._browsers = null` is no longer necessary since the new patch uses a proxy for get_browsers. That code has been removed in addTab and _linkBrowserToTab.

Regarding docShellIsActive, AFAICT the value of this is inconsequential until after the browser is instantiated.  So setting it where it is in _linkBrowserToTab seems appropriate.
(Assignee)

Updated

3 years ago
Attachment #8550902 - Attachment description: TabState.jsm: has code to avoid unnecessary browser instantiation on start-up → TabState.jsm: code for saving state of tabs with un-instantiated browser
(Assignee)

Comment 72

3 years ago
I have uploaded a new set of patches, which attempt to optimize start-up performance. These patches create tabs on session restore which do not instatiate the browser except on the selected tab. They also provide code changes which endeavor to keep the browser from being instantiated by other methods and listeners.  There is code to force browser instantiation when a tab is selected.  There is also code to handle proper saving of tab state on tabs which have not instantiated the browser.

Here is a more in-depth overview of the patches:

Revision of addTab/_linkBrowserToTab code: `linkedBrowser` getter has been changed to a proxy.  In this way, the linked browser is instantiated upon access to any given linkedBrowser property rather than access of linkedBrowser itself.  This allows for setting traps for certain properties where it is safe to return a value in place of the linkedBrowser property and avoid instantiating the browser.  An example of this would be linkedBrowser.currentURI.  In sessionstore, the url obtained from tabData is converted to an nsIURI object and set to the tab as tab.currentURI.  So when linkedBrowser.currentURI is accessed, tab.currentURI is returned instead of instantiating the browser.  One benefit of this can be seen when opening Preferences, or Add-ons page, where every tab is polled for currentURI until either finding a tab with that page already loaded, or getting to the end.

Also, the addition of `TabLinkBrowser` event to be used by listener in SessionStoreInternal.

Conversion of gBrowser.browsers which uses a proxy, rather than creating array _browsers of linkedBrowser(s).  (This may not now be necessary due to the way linkedBrowser proxy is implemented, nevertheless it is a worthwhile optimization in itself.)

Code changes in SessionStoreInternal -> restoreWindow/restoreTabs/restoreTab in such a way that tabs are not immediately restored but rather tabData is linked to the tab to be accessed when tab is later restored upon lazy browser instantiation.

Replaced TabOpen listener in SessionStoreInternal, which adds listeners to the linked browser, with TabLinkBrowser listener, so this occurs at the proper time.

Code change in SessionStoreInternal.onload to prevent early browser instantiation.

Change in NetworkPrioritizer.jsm TabOpen listener to prevent early browser instantiation.

Change in tabbox.xml selectedIndex setter to force linking of browser if tab is selected.

Changes in TabState.jsm -> _collectBaseTabData to properly deal with saving tab state on tabs with un-instantiated browsers.
I'll be away until February, so I unfortunately won't have time to look at your patches in the meantime.
(Assignee)

Comment 74

2 years ago
I just want to note that the optimizations in the current patches which prevent early browser instantiation affect only those sites which would cause instantiation during start-up.
(Assignee)

Comment 75

2 years ago
Created attachment 8561424 [details] [diff] [review]
bug_906076_PH5_2_tabbrowser.xml.diff

Updated patch for tabbrowser.xml, includes 2 bug fixes, and the addition of __SS_data trap for linkedBrowser proxy, which will return tab.__SS_data in its place.  This removes the necessity of TabState.jsm patch.
Attachment #8550898 - Attachment is obsolete: true
Attachment #8550902 - Attachment is obsolete: true
Flags: needinfo?(dteller)
(In reply to Allasso Travesser from comment #74)
> I just want to note that the optimizations in the current patches which
> prevent early browser instantiation affect only those sites which would
> cause instantiation during start-up.

What do you mean "affect"?
Flags: needinfo?(dteller)
(Assignee)

Comment 77

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #76)
> What do you mean "affect"?

Perhaps "are applied to" instead of "affect".

I mean that the only sites which have code changes in order to keep from instantiating the browser are those which would do so on start-up.
I assume that "site" == "call site", right? In this case, I believe that's exactly the right thing to do for a v1.
(Assignee)

Comment 79

2 years ago
yes.
I'm back, by the way. If you wish me to look at some of your patches, don't hesitate to r? or f? me.
(Assignee)

Updated

2 years ago
Attachment #8550899 - Flags: review?(dteller)
(Assignee)

Updated

2 years ago
Attachment #8550900 - Flags: review?(dteller)
(Assignee)

Updated

2 years ago
Attachment #8550901 - Flags: review?(dteller)
(Assignee)

Updated

2 years ago
Attachment #8561424 - Flags: review?(dteller)
(Assignee)

Comment 81

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #80)
> I'm back, by the way. If you wish me to look at some of your patches, don't
> hesitate to r? or f? me.

Thank you David. bug_906076_PH5_2_tabbrowser.xml.diff would be the most meaningful place to begin.
Comment on attachment 8561424 [details] [diff] [review]
bug_906076_PH5_2_tabbrowser.xml.diff

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

Lots of good things.

::: FF_9_ORIG/Contents/Resources/browser/omni/chrome/browser/content/browser/tabbrowser.xml
@@ +1657,5 @@
> +              referrerURI          = aParams.referrerURI;
> +              charset              = aParams.charset;
> +              postData             = aParams.postData;
> +            }
> +

I don't think you need this `if (aParams)`. In your code, `aParams` can never be undefined, right?

@@ +1675,5 @@
> +            let remote = !!aTab.remote;
> +
> +            if (!browser) {
> +              // No preloaded browser found, create one.
> +              browser = this._createBrowser({remote, aParams});

That call to `_createBrowser` looks weird. Shouldn't this be `{remote, uriIsAboutBlank}`?

@@ +1735,5 @@
> +                { flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT; }
> +              try {
> +                browser.loadURIWithFlags(aURI, flags, referrerURI, charset, postData);
> +              } catch (ex) {
> +                Cu.reportError(ex);

Nit: Could you take the opportunity to use Console.jsm's `console.error` instead?

@@ +1744,5 @@
> +            // activeness in the tab switcher.
> +            browser.docShellIsActive = false;
> +
> +            // notify sessionstore so listeners can be added to browser.
> +            // Originally sessionstore used TabOpen listener to do this.

Nit: That line of comment is not useful in the source code.

@@ +1747,5 @@
> +            // notify sessionstore so listeners can be added to browser.
> +            // Originally sessionstore used TabOpen listener to do this.
> +            let event = document.createEvent("Events");
> +            event.initEvent("TabLinkBrowser", true, false);
> +            aTab.dispatchEvent(event);

Why did you change the order? I think that in the original code, this happened before the "If we didn't swap docShells with a preloaded browser" block, right?

@@ +1750,5 @@
> +            event.initEvent("TabLinkBrowser", true, false);
> +            aTab.dispatchEvent(event);
> +
> +            // if we're not doing a window restore, and aTab is carrying
> +            // restore data, restore the tab.

That's a bit scary, as it puts part of Session Restore in a completely unrelated module. Why do you need to do it here?

@@ -1691,5 @@
>                }, 0, this.tabContainer);
>              }
>  
> -            // invalidate caches
> -            this._browsers = null;

You don't need to invalidate the cache anymore? How comes?

@@ +1842,5 @@
> +              
> +            // bug 906076 - we use a proxy to lazily instantiate the browser
> +            // and related elements, add them to DOM, and link the browser
> +            // to the tab.
> +            tab.linkedBrowser = new Proxy({},{

After carefully thinking this over, I confirm that using a Proxy here (rather than a simple lazy getter) makes sense, as it allows a few properties to be accessed without instantiating the <browser>, and then to instantiate lazily once this is really needed. You have comments to that effect, but they are a bit spread across the code, could you also group them together as a description of your Proxy technique?

@@ +1844,5 @@
> +            // and related elements, add them to DOM, and link the browser
> +            // to the tab.
> +            tab.linkedBrowser = new Proxy({},{
> +              get: (target, key) => {
> +                if (!tab._linkedBrowser) {

Is that `if` really necessary? Once `tab._linkedBrowser` is defined, you immediately overwrite `tab.linkedBrowser`, no?

@@ +1869,5 @@
> +                  }
> +                }
> +                // we use _linkedBrowser instead of linkedBrowser here because
> +                // _linkBrowserToTab has methods that get linkedBrowser before
> +                // returning, which would cause an error.

That sounds scary. Would it be possible to get rid of these methods?

@@ +1875,5 @@
> +                if (key in browser) {
> +                  let ret = browser[key];
> +                  if (typeof ret === "function") {
> +                    // returning function refs directly causes exception when
> +                    // called on certain interfaces

That comment doesn't really make sense. Something along the lines of "Make sure that methods use the correct `this`."

@@ +1876,5 @@
> +                  let ret = browser[key];
> +                  if (typeof ret === "function") {
> +                    // returning function refs directly causes exception when
> +                    // called on certain interfaces
> +                    ret = Function.prototype.bind.call(ret, browser);

`return ret.bind(browser);`

@@ +1880,5 @@
> +                    ret = Function.prototype.bind.call(ret, browser);
> +                  }
> +                  return ret;
> +                }
> +              },

Shouldn't you also provide a `set` hook that also instantiates the <browser>?

@@ +1920,2 @@
>                  this._lastRelatedTab.owner = null;
> +              }else{

Nit: here and everywhere else, `} else {` (with the whitespace).

@@ +1934,3 @@
>                }.bind(this));
>              }
> +            if (aURI && !uriIsAboutBlank) { tab.linkedBrowser.link; }

Nit:
`if ( ... ) {
   ...
}`

Also, what does this block do? This didn't appear in the original code, did it?

@@ +2735,5 @@
>                  readonly="true"/>
>  
>        <property name="browsers" readonly="true">
>         <getter>
>            <![CDATA[

Why do you need this change?

@@ +3326,5 @@
>            this.mCurrentTab._tPos = 0;
>            this.mCurrentTab._fullyOpen = true;
>            this.mCurrentTab.lastAccessed = Infinity;
>            this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;
> +          this.mCurrentTab._linkedBrowser = this.mCurrentBrowser;

Is that necessary?

@@ +3391,5 @@
>        <destructor>
>          <![CDATA[
>            for (var i = 0; i < this.mTabListeners.length; ++i) {
> +            // bug 906076 - listener won't be present on uninstantiated browsers
> +            if (!this.mTabListeners[i]) { continue; }

Nit: please use
 if (...) {
   continue;
 }

here and everywhere else.
Attachment #8561424 - Flags: review?(dteller) → feedback+
Comment on attachment 8550899 [details] [diff] [review]
SessionStore.jsm optimized to support lazy browser

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

::: FF_9_ORIG/Contents/Resources/browser/omni/modules/sessionstore/SessionStore.jsm
@@ +945,5 @@
>      // add tab change listeners to all already existing tabs
>      for (let i = 0; i < tabbrowser.tabs.length; i++) {
> +      let tab = tabbrowser.tabs[i];
> +      if (tab._linkedBrowser) {
> +        this.onTabLinkBrowser(aWindow, tab, true);

Checking a "_"-prefixed field of another class is not a good practice. I'd rather we expose a `if (tab.isLinked)`, or something along these lines.

@@ +2377,5 @@
>          tabbrowser.showTab(tabs[t]);
>          numVisibleTabs++;
>        }
> +      // if selected tab, instantiate and link the browser now
> +      if (t === selected) { tabs[t].linkedBrowser.link; }

I don't understand the changes in this function.

@@ +2398,5 @@
>      // If overwriting tabs, we want to reset each tab's "restoring" state. Since
>      // we're overwriting those tabs, they should no longer be restoring. The
>      // tabs will be rebuilt and marked if they need to be restored after loading
>      // state (in restoreTabs).
> +    if (overwriteTabs && openTabCount > 0) {

This `&& openTabCount > 0` doesn't do anything.

@@ +2399,5 @@
>      // we're overwriting those tabs, they should no longer be restoring. The
>      // tabs will be rebuilt and marked if they need to be restored after loading
>      // state (in restoreTabs).
> +    if (overwriteTabs && openTabCount > 0) {
> +      for (let i = 0; i < openTabCount; i++) {

I don't understand why you're changing this.

@@ +2404,3 @@
>          let tab = tabbrowser.tabs[i];
>          if (tabbrowser.browsers[i].__SS_restoreState)
> +          { this._resetTabRestoringState(tab); }

Nit: If you add braces, please match the style of the rest of the file.

@@ +2528,5 @@
>      }
>    },
>  
>    // Restores the given tab state for a given tab.
>    restoreTab(tab, tabData, options = {}) {

What do you gain by patching `restoreTab`? Won't it force instantation of `linkedBrowser` anyway?

@@ +2531,5 @@
>    // Restores the given tab state for a given tab.
>    restoreTab(tab, tabData, options = {}) {
>      let restoreImmediately = options.restoreImmediately;
>      let loadArguments = options.loadArguments;
> +    let browser = tab._linkedBrowser;

Again, inspecting in a `_`-prefixed field is not a good idea. Could you find another way to do this?
Attachment #8550899 - Flags: review?(dteller) → feedback+
Comment on attachment 8550900 [details] [diff] [review]
NetworkPrioritizer.jsm : has code to avoid unnecessary browser instantiation on start-up

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

::: FF_9_ORIG/Contents/Resources/browser/omni/modules/NetworkPrioritizer.jsm
@@ +50,5 @@
>      case "TabOpen":
> +      // don't unnecessarily instantiate browser on start-up - see bug 906076
> +      if (aEvent.target._linkedBrowser) { 
> +        BrowserHelper.onOpen(aEvent.target.linkedBrowser);
> +      }

That looks weird. Shouldn't you rather just replace `TabOpen` with `TabLinkedBrowser`? Otherwise, I fear that the module is not going to be initialized for all tabs.
Attachment #8550900 - Flags: review?(dteller) → feedback+
Comment on attachment 8550901 [details] [diff] [review]
tabbox.xml : has code to force instantiation of browser if tab is selected

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

::: FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/tabbox.xml
@@ +392,5 @@
>  
> +            // if tab is a tabbrowser-tab, make sure the browser is linked
> +            if (tab.classList.contains("tabbrowser-tab")) {
> +              tab.linkedBrowser.link;
> +            }

Normally, `tab.linkedBrowser` should work transparently. Why do we need a special case here?
Attachment #8550901 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 86

2 years ago
Thank you, David.  I am away from my workstation for a week, but when I get back I'll look into these issues in more detail.  I'll hold my comments for now.
(Assignee)

Comment 87

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #82)
> Comment on attachment 8561424 [details] [diff] [review]
> bug_906076_PH5_2_tabbrowser.xml.diff
> 
> :::
> FF_9_ORIG/Contents/Resources/browser/omni/chrome/browser/content/browser/
> tabbrowser.xml
> @@ +1657,5 @@
> > +              referrerURI          = aParams.referrerURI;
> > +              charset              = aParams.charset;
> > +              postData             = aParams.postData;
> > +            }
> > +
> 
> I don't think you need this `if (aParams)`. In your code, `aParams` can
> never be undefined, right?

yes, you're right.

> @@ +1675,5 @@
> > +            let remote = !!aTab.remote;
> > +
> > +            if (!browser) {
> > +              // No preloaded browser found, create one.
> > +              browser = this._createBrowser({remote, aParams});
> 
> That call to `_createBrowser` looks weird. Shouldn't this be `{remote,
> uriIsAboutBlank}`?
>
yes, you're right.
 
> @@ +1747,5 @@
> > +            // notify sessionstore so listeners can be added to browser.
> > +            // Originally sessionstore used TabOpen listener to do this.
> > +            let event = document.createEvent("Events");
> > +            event.initEvent("TabLinkBrowser", true, false);
> > +            aTab.dispatchEvent(event);
> 
> Why did you change the order? I think that in the original code, this
> happened before the "If we didn't swap docShells with a preloaded browser"
> block, right?
>
noted, will change that.
 
> @@ +1750,5 @@
> > +            event.initEvent("TabLinkBrowser", true, false);
> > +            aTab.dispatchEvent(event);
> > +
> > +            // if we're not doing a window restore, and aTab is carrying
> > +            // restore data, restore the tab.
> 
> That's a bit scary, as it puts part of Session Restore in a completely
> unrelated module. Why do you need to do it here?
>
I don't understand what you are saying here.

> @@ -1691,5 @@
> >                }, 0, this.tabContainer);
> >              }
> >  
> > -            // invalidate caches
> > -            this._browsers = null;
> 
> You don't need to invalidate the cache anymore? How comes?
> 
The `browsers` patch turns `_browsers` into a proxy.  Once it has been created there is no need to do it again.

> @@ +1842,5 @@
> > +              
> > +            // bug 906076 - we use a proxy to lazily instantiate the browser
> > +            // and related elements, add them to DOM, and link the browser
> > +            // to the tab.
> > +            tab.linkedBrowser = new Proxy({},{
> 
> After carefully thinking this over, I confirm that using a Proxy here
> (rather than a simple lazy getter) makes sense, as it allows a few
> properties to be accessed without instantiating the <browser>, and then to
> instantiate lazily once this is really needed. You have comments to that
> effect, but they are a bit spread across the code, could you also group them
> together as a description of your Proxy technique?
> 
Yes, I'll work on that.

> @@ +1844,5 @@
> > +            // and related elements, add them to DOM, and link the browser
> > +            // to the tab.
> > +            tab.linkedBrowser = new Proxy({},{
> > +              get: (target, key) => {
> > +                if (!tab._linkedBrowser) {
> 
> Is that `if` really necessary? Once `tab._linkedBrowser` is defined, you
> immediately overwrite `tab.linkedBrowser`, no?
> 
sure seems that way, yes, the `if` seems unnecessary.

> @@ +1869,5 @@
> > +                  }
> > +                }
> > +                // we use _linkedBrowser instead of linkedBrowser here because
> > +                // _linkBrowserToTab has methods that get linkedBrowser before
> > +                // returning, which would cause an error.
> 
> That sounds scary. Would it be possible to get rid of these methods?
> 
I will investigate that.

> @@ +1876,5 @@
> > +                  let ret = browser[key];
> > +                  if (typeof ret === "function") {
> > +                    // returning function refs directly causes exception when
> > +                    // called on certain interfaces
> > +                    ret = Function.prototype.bind.call(ret, browser);
> 
> `return ret.bind(browser);`
> 
noted, I will try that.

> @@ +1880,5 @@
> > +                    ret = Function.prototype.bind.call(ret, browser);
> > +                  }
> > +                  return ret;
> > +                }
> > +              },
> 
> Shouldn't you also provide a `set` hook that also instantiates the <browser>?
>
seems like it, I will do that.
 
> Also, what does this block do? This didn't appear in the original code, did
> it?
> 
> @@ +2735,5 @@
> >                  readonly="true"/>
> >  
> >        <property name="browsers" readonly="true">
> >         <getter>
> >            <![CDATA[
> 
> Why do you need this change?
> 
The original code would force browser instantiation on every single tab for any get of `browsers`. This patch keeps it from doing that.

> @@ +3326,5 @@
> >            this.mCurrentTab._tPos = 0;
> >            this.mCurrentTab._fullyOpen = true;
> >            this.mCurrentTab.lastAccessed = Infinity;
> >            this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;
> > +          this.mCurrentTab._linkedBrowser = this.mCurrentBrowser;
> 
> Is that necessary?
> 
_linkedBrowser gets tested in SessionStore.jsm.  I suppose this can change when applying correction based on your comment regarding SessionStore.jsm patch
(Assignee)

Comment 88

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #83)
> Comment on attachment 8550899 [details] [diff] [review]
> SessionStore.jsm optimized to support lazy browser
> 
> Review of attachment 8550899 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> FF_9_ORIG/Contents/Resources/browser/omni/modules/sessionstore/SessionStore.
> jsm
> @@ +945,5 @@
> >      // add tab change listeners to all already existing tabs
> >      for (let i = 0; i < tabbrowser.tabs.length; i++) {
> > +      let tab = tabbrowser.tabs[i];
> > +      if (tab._linkedBrowser) {
> > +        this.onTabLinkBrowser(aWindow, tab, true);
> 
> Checking a "_"-prefixed field of another class is not a good practice. I'd
> rather we expose a `if (tab.isLinked)`, or something along these lines.
> 
Okay, perhaps we can create a getter eg `isLinked` which will return `!!_linkedBrowser`?

> @@ +2377,5 @@
> >          tabbrowser.showTab(tabs[t]);
> >          numVisibleTabs++;
> >        }
> > +      // if selected tab, instantiate and link the browser now
> > +      if (t === selected) { tabs[t].linkedBrowser.link; }
> 
> I don't understand the changes in this function.
> 
I'll investigate

> @@ +2398,5 @@
> >      // If overwriting tabs, we want to reset each tab's "restoring" state. Since
> >      // we're overwriting those tabs, they should no longer be restoring. The
> >      // tabs will be rebuilt and marked if they need to be restored after loading
> >      // state (in restoreTabs).
> > +    if (overwriteTabs && openTabCount > 0) {
> 
> This `&& openTabCount > 0` doesn't do anything.
> 
indeed...

> @@ +2399,5 @@
> >      // we're overwriting those tabs, they should no longer be restoring. The
> >      // tabs will be rebuilt and marked if they need to be restored after loading
> >      // state (in restoreTabs).
> > +    if (overwriteTabs && openTabCount > 0) {
> > +      for (let i = 0; i < openTabCount; i++) {
> 
> I don't understand why you're changing this.
> 
don't remember, I'll investigate

> @@ +2528,5 @@
> >      }
> >    },
> >  
> >    // Restores the given tab state for a given tab.
> >    restoreTab(tab, tabData, options = {}) {
> 
> What do you gain by patching `restoreTab`? Won't it force instantation of
> `linkedBrowser` anyway?
> 
The whole point is to *not* instantiate, thus the patch of restoreTab.  Perhaps the distinction you are missing is `let browser = tab._linkedBrowser;` rather than `let browser = tab.linkedBrowser;`.  But it sounds like this will all have to be reworked anyway.
(Assignee)

Comment 89

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #84)
> Comment on attachment 8550900 [details] [diff] [review]
> NetworkPrioritizer.jsm : has code to avoid unnecessary browser instantiation
> on start-up
> 
> Review of attachment 8550900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: FF_9_ORIG/Contents/Resources/browser/omni/modules/NetworkPrioritizer.jsm
> @@ +50,5 @@
> >      case "TabOpen":
> > +      // don't unnecessarily instantiate browser on start-up - see bug 906076
> > +      if (aEvent.target._linkedBrowser) { 
> > +        BrowserHelper.onOpen(aEvent.target.linkedBrowser);
> > +      }
> 
> That looks weird. Shouldn't you rather just replace `TabOpen` with
> `TabLinkedBrowser`? Otherwise, I fear that the module is not going to be
> initialized for all tabs.

indeed.
(Assignee)

Comment 90

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #85)
> Comment on attachment 8550901 [details] [diff] [review]
> tabbox.xml : has code to force instantiation of browser if tab is selected
> 
> Review of attachment 8550901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/
> tabbox.xml
> @@ +392,5 @@
> >  
> > +            // if tab is a tabbrowser-tab, make sure the browser is linked
> > +            if (tab.classList.contains("tabbrowser-tab")) {
> > +              tab.linkedBrowser.link;
> > +            }
> 
> Normally, `tab.linkedBrowser` should work transparently. Why do we need a
> special case here?

This insures that when user clicks on tab, the browser will be instantiated.  Also in case when tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).

Probably would be simpler though to just do `tab.linkedBrowser ?? tab.linkedBrowser.link`.
(Assignee)

Comment 91

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #82)
> Comment on attachment 8561424 [details] [diff] [review]
> bug_906076_PH5_2_tabbrowser.xml.diff
> 
> Review of attachment 8561424 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +1869,5 @@
> > +                  }
> > +                }
> > +                // we use _linkedBrowser instead of linkedBrowser here because
> > +                // _linkBrowserToTab has methods that get linkedBrowser before
> > +                // returning, which would cause an error.
> 
> That sounds scary. Would it be possible to get rid of these methods?
> 
TabLinkBrowser listener in SessionStore.jsm adds event listeners to tab.linkedBrowser, which must be done at some point.  I think the issue then becomes the timing of when this is handled.

Currently in _linkBrowserToTab, a property _linkedBrowser is set to the <browser> shortly after <browser> is created. _linkedBrowser property is then used after _linkBrowserToTab returns and linkedBrowser is set to that. Since we're handing the whole thing over to linkedBrowser anyway (in place of the proxy) perhaps it would make more sense to just set linkedBrowser forthright in _linkBrowserToTab (rather than waiting until linkBrowserToTab returns), and probably eliminate the _linkedBrowser go-between altogether.  As long as the strategy of replacing the proxy with the <browser> once it is created is being used, this seems to make more sense to me now, and at the moment I don't foresee any problems with eliminating _linkedBrowser if this is the case.  It would also be more congruent with the original code anyway.  This may help smooth out some of the code in other places as well.

We still may have to set/unset some property eg tab.isLinked for a few special cases, but perhaps some means will become apparent which will allow a more elegant solution in those cases.
(Assignee)

Comment 92

2 years ago
...eg, line 1690:

aTab._linkedBrowser = browser;

becomes

aTab.linkedBrowser = browser;
(In reply to Allasso Travesser from comment #87)
> > @@ +1750,5 @@
> > > +            event.initEvent("TabLinkBrowser", true, false);
> > > +            aTab.dispatchEvent(event);
> > > +
> > > +            // if we're not doing a window restore, and aTab is carrying
> > > +            // restore data, restore the tab.
> > 
> > That's a bit scary, as it puts part of Session Restore in a completely
> > unrelated module. Why do you need to do it here?
> >
> I don't understand what you are saying here.

Apparently, in this change, you are taking a little bit from the code of Session Restore and putting it in tabbrowser.xml. That sounds like a recipe for accidents.

> > You don't need to invalidate the cache anymore? How comes?
> > 
> The `browsers` patch turns `_browsers` into a proxy.  Once it has been
> created there is no need to do it again.

Can you walk me through that?

> > Also, what does this block do? This didn't appear in the original code, did
> > it?

I don't think you answered that.

> > @@ +2735,5 @@
> > >                  readonly="true"/>
> > >  
> > >        <property name="browsers" readonly="true">
> > >         <getter>
> > >            <![CDATA[
> > 
> > Why do you need this change?
> > 
> The original code would force browser instantiation on every single tab for
> any get of `browsers`. This patch keeps it from doing that.

That probably makes sense. In which cases do we get `browsers`?

> 
> > @@ +3326,5 @@
> > >            this.mCurrentTab._tPos = 0;
> > >            this.mCurrentTab._fullyOpen = true;
> > >            this.mCurrentTab.lastAccessed = Infinity;
> > >            this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;
> > > +          this.mCurrentTab._linkedBrowser = this.mCurrentBrowser;
> > 
> > Is that necessary?
> > 
> _linkedBrowser gets tested in SessionStore.jsm.  I suppose this can change
> when applying correction based on your comment regarding SessionStore.jsm
> patch

I'd prefer, yes.

(In reply to Allasso Travesser from comment #88)
> > Checking a "_"-prefixed field of another class is not a good practice. I'd
> > rather we expose a `if (tab.isLinked)`, or something along these lines.
> > 
> Okay, perhaps we can create a getter eg `isLinked` which will return
> `!!_linkedBrowser`?

I'd prefer `hasLinkedBrowser`, which is less open to misinterpretation. Anyway, in the future (but before we land your code) we can decide to change the name if we find something better.

> > @@ +2528,5 @@
> > What do you gain by patching `restoreTab`? Won't it force instantation of
> > `linkedBrowser` anyway?
> > 
> The whole point is to *not* instantiate, thus the patch of restoreTab. 
> Perhaps the distinction you are missing is `let browser =
> tab._linkedBrowser;` rather than `let browser = tab.linkedBrowser;`.  But it
> sounds like this will all have to be reworked anyway.

Let me rephrase: is there a case in which `restoreTab` can be expected to do its work without instantiating `linkedBrowser`?

> This insures that when user clicks on tab, the browser will be instantiated.  Also in case when 
> tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).

But what happens if you do not call `link` yourself at this line?

> Probably would be simpler though to just do `tab.linkedBrowser ?? tab.linkedBrowser.link`.

I'm not a big fan of `tab.linkedBrowser.link`. That's a hack, as there is no such field/method in the code. I'd rather have a method `tab.ensureLinkedBrowser()` or `tab.linkedBrowser.ensureLinked()`.
(Assignee)

Comment 94

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #93)
> > The `browsers` patch turns `_browsers` into a proxy.  Once it has been
> > created there is no need to do it again.
> 
> Can you walk me through that?

Hmm, I wonder if we're connecting on this.

In the original code `browsers` created an array `_browsers` of the linkedBrowser for each tab. The key for each browser matched the key in `tabs` for its corresponding tab.  Anytime  something happens which would alter `tabs`, this array had to be recreated.  This was done by setting `_browsers` to null (invalidate cache) which would tell `browsers` to recreate the  `_browsers` array on the next get in order to re-synchronize.

The `browsers` patch turns `_browsers` into a proxy instead which will return the linkedBrowser for the tab of the corresponding key in `tabs`.  No synchronization is necessary, the proxy just does the work, thus there is no need to invalidate.  

> > +            if (aURI && !uriIsAboutBlank) { tab.linkedBrowser.link; }
>
> Also, what does this block do? This didn't appear in the original code, did it?
> I don't think you answered that.

If a URI is provided to `addTab`, it is assumed it wants to be loaded.  Therefore the browser has to be instantiated.  In future patch the instantiation code in the function block will change.

> That probably makes sense. In which cases do we get `browsers`?

On startup:
FullZoom_init@chrome://browser/content/browser.js:2283:1
NP_WH_decreasePriority@resource:///modules/NetworkPrioritizer.jsm:163:5
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:2353:9
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:2423:1
getBrowserAtIndex@chrome://browser/content/tabbrowser.xml:345:13

> Let me rephrase: is there a case in which `restoreTab` can be expected to do
> its work without instantiating `linkedBrowser`?

AFAICT not in the original code, but I may not be understanding your question.

> > > > FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/
> > > > tabbox.xml
> > > > @@ +392,5 @@
> > > >  
> > > > +            // if tab is a tabbrowser-tab, make sure the browser is linked
> > > > +            if (tab.classList.contains("tabbrowser-tab")) {
> > > > +              tab.linkedBrowser.link;
> > > > +            }
> > > 
> > > Normally, `tab.linkedBrowser` should work transparently. Why do we need a
> > > special case here?
> >
> > This insures that when user clicks on tab, the browser will be instantiated.  Also in case when 
> > tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).
> 
> But what happens if you do not call `link` yourself at this line?

In this method there is a sequence of calls which eventually fires a "select" event, which is heard by tabbrowser.  This is what calls updateCurrentBrowser. If the browser is not instantiated by this point, there is no linkedPanel, thus the code will not run and browser will be neither instantiated nor updated.

***

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #82)
> > > @@ +1844,5 @@
> > > +            // and related elements, add them to DOM, and link the browser
> > > +            // to the tab.
> > > +            tab.linkedBrowser = new Proxy({},{
> > > +              get: (target, key) => {
> > > +                if (!tab._linkedBrowser) {
> > 
> > Is that `if` really necessary? Once `tab._linkedBrowser` is defined, you
> > immediately overwrite `tab.linkedBrowser`, no?
> > 
> sure seems that way, yes, the `if` seems unnecessary.

I ammend that : Yes, the test is necessary. If tab.linkedBrowser is referenced before the browser is instantiated, then the reference is used to access properties/methods, the reference will still be pointing to the proxy even after tab.linkeBrowser is updated to reference the <browser>. So:

let browser = tab.linkedBrowser;
browser.permanentKey;
browser.permanentKey;
browser.permanentKey;

will access the proxy and call _linkBrowserToTab three times, even though tab.linkedBrowser is no longer pointing to the proxy after the first time.

***

In my WIP I have eliminated _linkedBrowser and reworked the proxy/linkedBrowser strategy as I suggested in comment 91.  I have added the `hasLinkedBrowser` property as you suggested.
(Assignee)

Comment 95

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #83)
> @@ +2377,5 @@
> >          tabbrowser.showTab(tabs[t]);
> >          numVisibleTabs++;
> >        }
> > +      // if selected tab, instantiate and link the browser now
> > +      if (t === selected) { tabs[t].linkedBrowser.link; }
> 
> I don't understand the changes in this function.
>
If the tab will be the selected tab, it will be loaded and will first need to be instantiated.

Though this will change to:

if (t === selected) {
  tabs[t].ensureLinkedBrowser();
}
 
> @@ +2398,5 @@
> >      // If overwriting tabs, we want to reset each tab's "restoring" state. Since
> >      // we're overwriting those tabs, they should no longer be restoring. The
> >      // tabs will be rebuilt and marked if they need to be restored after loading
> >      // state (in restoreTabs).
> > +    if (overwriteTabs && openTabCount > 0) {
> 
> This `&& openTabCount > 0` doesn't do anything.
> 
> @@ +2399,5 @@
> >      // we're overwriting those tabs, they should no longer be restoring. The
> >      // tabs will be rebuilt and marked if they need to be restored after loading
> >      // state (in restoreTabs).
> > +    if (overwriteTabs && openTabCount > 0) {
> > +      for (let i = 0; i < openTabCount; i++) {
> 
> I don't understand why you're changing this.
> 
I don't know what I was thinking there.  I believe this should be something like:

if (overwriteTabs) {
  for (let i = 0; i < tabbrowser.tabs.length; i++) {
    let tab = tabbrowser.tabs[i];
    if (tab.hasLinkedBrowser && tab.linkedBrowser.__SS_restoreState) {
      this._resetTabRestoringState(tab);
    }
  }
}
(Assignee)

Updated

2 years ago
Flags: needinfo?(dteller)
(In reply to Allasso Travesser from comment #94)
> (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment
> #93)
> > > The `browsers` patch turns `_browsers` into a proxy.  Once it has been
> > > created there is no need to do it again.
> > 
> > Can you walk me through that?
> 
> Hmm, I wonder if we're connecting on this.
> 
> In the original code `browsers` created an array `_browsers` of the
> linkedBrowser for each tab. The key for each browser matched the key in
> `tabs` for its corresponding tab.  Anytime  something happens which would
> alter `tabs`, this array had to be recreated.  This was done by setting
> `_browsers` to null (invalidate cache) which would tell `browsers` to
> recreate the  `_browsers` array on the next get in order to re-synchronize.
> 
> The `browsers` patch turns `_browsers` into a proxy instead which will
> return the linkedBrowser for the tab of the corresponding key in `tabs`.  No
> synchronization is necessary, the proxy just does the work, thus there is no
> need to invalidate.

Ok, I'm starting to understand the change.
1/ Why do you need to handle numbers and non-numbers differently?
2/ What is the objective of this change? Is it a cleanup, or is actually necessary?

> 
> > > +            if (aURI && !uriIsAboutBlank) { tab.linkedBrowser.link; }
> >
> > Also, what does this block do? This didn't appear in the original code, did it?
> > I don't think you answered that.
> 
> If a URI is provided to `addTab`, it is assumed it wants to be loaded. 
> Therefore the browser has to be instantiated.  In future patch the
> instantiation code in the function block will change.

And won't the browser be instantiated automatically at some point?

> 
> > That probably makes sense. In which cases do we get `browsers`?
> 
> On startup:
> FullZoom_init@chrome://browser/content/browser.js:2283:1
> NP_WH_decreasePriority@resource:///modules/NetworkPrioritizer.jsm:163:5
> ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:2353:9
> ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:2423:1
> getBrowserAtIndex@chrome://browser/content/tabbrowser.xml:345:13

Ok, makes sense.

> > Let me rephrase: is there a case in which `restoreTab` can be expected to do
> > its work without instantiating `linkedBrowser`?
> 
> AFAICT not in the original code, but I may not be understanding your
> question.

As far as I can tell, by definition, no matter how much optimization you put in `restoreTab`, it will always need to instantiate `linkedBrowser`, no? If I am right, you probably shouldn't spend time trying to optimize `restoreTab`.

> 
> > > > > FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/
> > > > > tabbox.xml
> > > > > @@ +392,5 @@
> > > > >  
> > > > > +            // if tab is a tabbrowser-tab, make sure the browser is linked
> > > > > +            if (tab.classList.contains("tabbrowser-tab")) {
> > > > > +              tab.linkedBrowser.link;
> > > > > +            }
> > > > 
> > > > Normally, `tab.linkedBrowser` should work transparently. Why do we need a
> > > > special case here?
> > >
> > > This insures that when user clicks on tab, the browser will be instantiated.  Also in case when 
> > > tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).
> > 
> > But what happens if you do not call `link` yourself at this line?
> 
> In this method there is a sequence of calls which eventually fires a
> "select" event, which is heard by tabbrowser.  This is what calls
> updateCurrentBrowser. If the browser is not instantiated by this point,
> there is no linkedPanel, thus the code will not run and browser will be
> neither instantiated nor updated.

Ok. Please document this. Also, as mentioned somewhere else, I believe you should find something nicer than `link`.

> 
> ***
> 
> (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment
> #82)
> > > Is that `if` really necessary? Once `tab._linkedBrowser` is defined, you
> > > immediately overwrite `tab.linkedBrowser`, no?
> > > 
> > sure seems that way, yes, the `if` seems unnecessary.
> 
> I ammend that : Yes, the test is necessary.
[...]

Ok, this makes sense. Please document this.

> 
> In my WIP I have eliminated _linkedBrowser and reworked the
> proxy/linkedBrowser strategy as I suggested in comment 91.  I have added the
> `hasLinkedBrowser` property as you suggested.

Thanks.
Flags: needinfo?(dteller)
(In reply to Allasso Travesser from comment #95)
> If the tab will be the selected tab, it will be loaded and will first need
> to be instantiated.
> 
> Though this will change to:
> 
> if (t === selected) {
>   tabs[t].ensureLinkedBrowser();
> }

Ok, please docuement it.


> > @@ +2399,5 @@
> > >      // we're overwriting those tabs, they should no longer be restoring. The
> > >      // tabs will be rebuilt and marked if they need to be restored after loading
> > >      // state (in restoreTabs).
> > > +    if (overwriteTabs && openTabCount > 0) {
> > > +      for (let i = 0; i < openTabCount; i++) {
> > 
> > I don't understand why you're changing this.
> > 
> I don't know what I was thinking there.  I believe this should be something
> like:
> 
> if (overwriteTabs) {
>   for (let i = 0; i < tabbrowser.tabs.length; i++) {
>     let tab = tabbrowser.tabs[i];
>     if (tab.hasLinkedBrowser && tab.linkedBrowser.__SS_restoreState) {
>       this._resetTabRestoringState(tab);
>     }
>   }
> }

Mmmh... I don't remember __SS_restoreState well enough. This is something you should see with ttaubert.
(Assignee)

Comment 98

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #96)
> (In reply to Allasso Travesser from comment #94)
> > (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment
> > #93)
> > > > The `browsers` patch turns `_browsers` into a proxy.  Once it has been
> > > > created there is no need to do it again.
> > > 
> > > Can you walk me through that?
> > 
> > Hmm, I wonder if we're connecting on this.
> > 
> > In the original code `browsers` created an array `_browsers` of the
> > linkedBrowser for each tab. The key for each browser matched the key in
> > `tabs` for its corresponding tab.  Anytime  something happens which would
> > alter `tabs`, this array had to be recreated.  This was done by setting
> > `_browsers` to null (invalidate cache) which would tell `browsers` to
> > recreate the  `_browsers` array on the next get in order to re-synchronize.
> > 
> > The `browsers` patch turns `_browsers` into a proxy instead which will
> > return the linkedBrowser for the tab of the corresponding key in `tabs`.  No
> > synchronization is necessary, the proxy just does the work, thus there is no
> > need to invalidate.
> 
> Ok, I'm starting to understand the change.
> 1/ Why do you need to handle numbers and non-numbers differently?
> 2/ What is the objective of this change? Is it a cleanup, or is actually
> necessary?

1/ Non-string will throw on RegExp.test(...) in that function.  This can happen when an iterator is used, and `type name` is symbol.

2/ It is essential or else browser will be instantiated on every tab on the first get of browsers, even if it is for a single member.

> > 
> > > > +            if (aURI && !uriIsAboutBlank) { tab.linkedBrowser.link; }
> > >
> > > Also, what does this block do? This didn't appear in the original code, did it?
> > > I don't think you answered that.
> > 
> > If a URI is provided to `addTab`, it is assumed it wants to be loaded. 
> > Therefore the browser has to be instantiated.  In future patch the
> > instantiation code in the function block will change.
> 
> And won't the browser be instantiated automatically at some point?

Apparently not.

> > > Let me rephrase: is there a case in which `restoreTab` can be expected to do
> > > its work without instantiating `linkedBrowser`?
> > 
> > AFAICT not in the original code, but I may not be understanding your
> > question.
> 
> As far as I can tell, by definition, no matter how much optimization you put
> in `restoreTab`, it will always need to instantiate `linkedBrowser`, no? If
> I am right, you probably shouldn't spend time trying to optimize
> `restoreTab`.

Okay, I think I see what you are saying now.  So then rather than doing `if`s in restoreTab, I should create a separate method for dealing with tabs which we don't want to restore and move the `if` upstream.

> > 
> > > > > > FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/
> > > > > > tabbox.xml
> > > > > > @@ +392,5 @@
> > > > > >  
> > > > > > +            // if tab is a tabbrowser-tab, make sure the browser is linked
> > > > > > +            if (tab.classList.contains("tabbrowser-tab")) {
> > > > > > +              tab.linkedBrowser.link;
> > > > > > +            }
> > > > > 
> > > > > Normally, `tab.linkedBrowser` should work transparently. Why do we need a
> > > > > special case here?
> > > >
> > > > This insures that when user clicks on tab, the browser will be instantiated.  Also in case when 
> > > > tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).
> > > 
> > > But what happens if you do not call `link` yourself at this line?
> > 
> > In this method there is a sequence of calls which eventually fires a
> > "select" event, which is heard by tabbrowser.  This is what calls
> > updateCurrentBrowser. If the browser is not instantiated by this point,
> > there is no linkedPanel, thus the code will not run and browser will be
> > neither instantiated nor updated.
> 
> Ok. Please document this. Also, as mentioned somewhere else, I believe you
> should find something nicer than `link`.

I am using `ensureLinkedBrowser` now.
(Assignee)

Comment 99

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #96)
> > As far as I can tell, by definition, no matter how much optimization you put
> > in `restoreTab`, it will always need to instantiate `linkedBrowser`, no? If
> > I am right, you probably shouldn't spend time trying to optimize
> > `restoreTab`.
> 
> Okay, I think I see what you are saying now.  So then rather than doing
> `if`s in restoreTab, I should create a separate method for dealing with tabs
> which we don't want to restore and move the `if` upstream.

As I look at this more, maybe that isn't a good idea.  It would require duplicating a lot of code.  It seems there needs to be two levels of "restoring", one if for tabs with and and one for tabs without instantiated browsers.

Perhaps the solution is to rename/redefine that method?
Flags: needinfo?(dteller)
I'm still not sure I understand the `restoreTab` stuff. If my memory serves correctly, by definition, restore tab needs to restore a tab, which cannot be done without linking the browser, right? Or is there a case in which we can restore a tab without linking the browser?
Flags: needinfo?(dteller)
(Assignee)

Comment 101

2 years ago
In my patch, I have used `if`s in restoreTab to determine whether to actually restore the tab (tab.hasLinkedBrowser) or do a partial restore (!tab.hasLinkedBrowser).

In the partial restore (roughly):

1/ All of the properties explicitly set on the tab (__SS_extdata, pinned/hidden state, lastAccessed, etc) are still set

2/ Two properties which get set in the SessionStore:restoreHistory process (label and image) are set explicitly from tabData

3/ The SessionStore:restoreHistory event is *not* fired which would complete the restore

4/ A property tab.__SS_data is set to reference tabData to be accessed later when finally doing a full restore once the browser is instantiated*.  

I don't know of any other way to accomplish this in order to not instantiate/link the browser upon window restore, without duplicating a lot of code.  Like I said, perhaps the solution is to rename/redefine restoreTab.

*In the current patch the restore upon instantiation is done with a direct call to SessionStore.setTabState in _linkBrowserToTab(), however in my WIP this is done in SessionStore from TabLinkBrowser listener.  At this point the browser is linked, tab.hasLinkedBrowser is set, and restoreTab will follow the full restore route.
(Assignee)

Comment 102

2 years ago
Hello David, when you have time I look forward to your input regarding redefining/renaming restoreTab as talked about in comment 101.

Thanks
Flags: needinfo?(dteller)
(Assignee)

Comment 103

2 years ago
Just to be clear, here is a rough sequence of events of restore:

1/ tab is created but browser is not instantiated.
2/ SS.restoreTab called on tab to do a partial restore (see comment 101)
3/ sometime in the future, the tab needs to be loaded at which point linkedBrowser.<property> is accessed
4/ browser is instantiated
5/ SS.restoreTab called on tab to do a full restore
6/ tab is loaded
Ok, got it. Currently, `restoreTab` is called by:
- `setTabState` [b]
- `duplicateTab` [b];
- `reviveCrashedTab` called alone from browser.js [c];
- `reviveCrashedTab` called in batch from browser.js [a];
- `restoreTabs` [a].

[a] Doing it lazily would be very useful.
[b] Doing it lazily makes sense, but can be kept for a followup bug.
[c] Doing it lazily makes no sense.

I am now officially sold to the idea that `restoreTab` should be done lazily whenever possible. We probably don't want to rename the function, just to document the fact that it attempts to restore the tab lazily whenever possible.

As much as possible, the implementation of lazy restoration should be self-contained in the implementation of `restoreTab` (and possibly an external WeakMap if necessary), without spilling on other functions/methods/modules.

Are we good on that?
Flags: needinfo?(dteller)
(Assignee)

Comment 105

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #104)
> ...Are we good on that?

Sounds good.  I will get to work on finishing up the next phase of patches with this direction in mind.
I'm anxious to see this completed :)
Any news?
Flags: needinfo?(allassopraise)
(Assignee)

Comment 107

2 years ago
Created attachment 8587563 [details] [diff] [review]
tabbrowser.xml - lazy browser, proxy `browsers`

updates from last patch:

Removed _linkedBrowser "go-between" property and moved linkedBrowser referencing to linkBrowserToTab()
Added tab.hasLinkedBrowser property
Added tab.ensureLinkedBrowser method
Moved TabLinkBrowser event dispatch to before swap docShells in linkBrowserToTab()
Removed setTabState call in linkBrowserToTab (now dealt with in SessionStore.jsm)
Added code in linkedBrowser proxy to instantiate browser when setting property on linkedBrowser
Changed code in linkedBrowser proxy for better implementation when accessed property is a function

Improved documentation
Cleaned up code

Please give feedback on `this.mCurrentTab.ensureLinkedBrowser = function() {}` in tabbrowser <constuctor> - keeps things from breaking, but seems like a bit of a hack, no?
Attachment #8561424 - Attachment is obsolete: true
Flags: needinfo?(allassopraise)
Attachment #8587563 - Flags: feedback?(dteller)
(Assignee)

Comment 108

2 years ago
Created attachment 8587571 [details] [diff] [review]
SessionStore.jsm optimized to support lazy browser

updates from last patch:

Added code in onTabLinkBrowser() to fully restore the tab
Removed test for selected tab from restoreWindow
Moved all restore code to restoreTab
Attachment #8550899 - Attachment is obsolete: true
Attachment #8587571 - Flags: feedback?(dteller)
(Assignee)

Comment 109

2 years ago
Created attachment 8587574 [details] [diff] [review]
PH_6_NetworkPrioritizer.jsm.diff

updates from last patch:

Changed `handle "TabOpen" > test for browser instantiated` to `handle TabLinkBrowser`
Attachment #8550900 - Attachment is obsolete: true
Attachment #8587574 - Flags: feedback?(dteller)
(Assignee)

Comment 110

2 years ago
Created attachment 8587576 [details] [diff] [review]
tabbox.xml.diff

updates from last patch:

Changed tab.linkedBrowser.link to tab.ensureLinkedBrowser()
Attachment #8550901 - Attachment is obsolete: true
Attachment #8587576 - Flags: feedback?(dteller)
(Assignee)

Comment 111

2 years ago
Also on SessionStore.jsm patch (attachment 8587576 [details] [diff] [review]):

Added documentation to describe the optimized restoreTab() process.
Cleaned up code.
Alasso, do you have access to Reviewboard (https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/review-requests.html)? If so, I suggest we move discussions over there.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 113

2 years ago
I don't have a Mozilla-registered LDAP account, so I am not able to complete the SSH configuration (AFAICT).  Other than that it is (should be) configured.
Flags: needinfo?(allassopraise) → needinfo?(dteller)
Comment on attachment 8587563 [details] [diff] [review]
tabbrowser.xml - lazy browser, proxy `browsers`

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

::: FF_9_ORIG/Contents/Resources/browser/omni/chrome/browser/content/browser/tabbrowser.xml
@@ +1819,5 @@
> +            // linkedBrowser.currentURI by the proxy when we don't need to
> +            // instantiate the browser. This is useful for actions such as
> +            // opening preferences, which polls every tab for URIs and thus
> +            // could incidently instantiate the browser for every tab.
> +            tab.currentURI = Services.io.newURI(aURI,null,null);

No, that's not a good place to attach it. If the user navigates away and a client requests `tab.currentURI`, the client will get the original version of `currentURI` which is not the current uri anymore.

Rather, you should so something along the lines of
  let originalURI = Services.io.newURI(...);

// the `get` method of the proxy:
  `if (key === "currentURI") {
     return originalURI;
   }`

@@ +1839,5 @@
> +            // the browser, and dump the proxy.  This re-referencing is done
> +            // in _linkBrowserToTab - before other methods are called on
> +            // linkedBrowser which would otherwise cause recursion.
> +            tab.linkedBrowser = new Proxy({},{
> +              "get": (target, key) => {

Nit: just `get`, without the backquotes.

@@ +1844,5 @@
> +                if (!tab.hasLinkedBrowser) {
> +                  if (key === "currentURI") {
> +                    return tab.currentURI;
> +                  } else if (key === "__SS_data" && tab.__SS_data) {
> +                    return tab.__SS_data;

Could you rather change SessionStore.jsm to store `__SS_data` somewhere other than in the <browser>? For instance, in a WeakMap associating the <browser> to an object.

@@ +1867,5 @@
> +                tab.linkedBrowser[key] = value;
> +              }
> +            });
> +            
> +            tab.ensureLinkedBrowser = function() {

This method needs documentation, especially since it's now public.

@@ +2737,5 @@
> +                  if (typeof name === "string") {
> +                    // test for string because if called
> +                    // by an iterator, this will croak.
> +                    let obj = /^(\d+|length)$/.test(name) ? this.tabs : target;
> +                    return /^\d+$/.test(name) ? obj[name].linkedBrowser : obj[name];

I think it would be clearer as follows:

// This property implements an array of `linkedBrowser` for all tabs.
// However, for performance sake, we try to avoid actually calling
// `linkedBrowser` until this is necessary.
if (name == "length") {
  return this.tabs.length;
}
if (Number.isInteger(name)) {
  return this.tabs[name].linkedBrowser;
}
// Anything else is access to a prototype method
// (e.g. `map`, `filter`, etc.)
return target[name];

@@ +3322,5 @@
>            this.mCurrentTab.lastAccessed = Infinity;
>            this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;
> +          this.mCurrentTab.hasLinkedBrowser = true;
> +          // we know the browser is linked, so don't do anything
> +          this.mCurrentTab.ensureLinkedBrowser = function() {};

Shouldn't we set this as part of `_linkBrowserToTab`?
Attachment #8587563 - Flags: feedback?(dteller) → feedback+
Comment on attachment 8587571 [details] [diff] [review]
SessionStore.jsm optimized to support lazy browser

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

::: FF_9_ORIG/Contents/Resources/browser/omni/modules/sessionstore/SessionStore.jsm
@@ +941,5 @@
>      }
>  
>      var tabbrowser = aWindow.gBrowser;
>  
>      // add tab change listeners to all already existing tabs

(could you add a comment explaining when we'll add the listeners to tabs that have not been created or linked yet?)

@@ +1317,4 @@
>      let browser = aTab.linkedBrowser;
>      browser.addEventListener("SwapDocShells", this);
>      browser.addEventListener("oop-browser-crashed", this);
> +    if (aTab.__SS_data) {

This needs documentation. Also, I'd prefer if you got rid of `__SS_data` and put this in a global WeakMap with a more understandable name.

@@ +1324,5 @@
> +      } else {
> +        delete aTab.__SS_data.pinned;
> +      }
> +      aTab.__SS_data.hidden = aTab.hidden;
> +      let state = JSON.stringify(aTab.__SS_data);

That looks expensive. Is this something that we already did?

@@ +2415,2 @@
>            this._resetTabRestoringState(tab);
> +        }

Can you document why we don't need to do this for tabs that are not linked yet?
Attachment #8587571 - Flags: feedback?(dteller) → feedback+
Actually, I realize that moving __SS_data to its own WeakMap might be a big change. Perhaps that's something we wish to land separately.
Flags: needinfo?(dteller)
Attachment #8587574 - Flags: feedback?(dteller) → feedback+
Comment on attachment 8587576 [details] [diff] [review]
tabbox.xml.diff

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

::: FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/tabbox.xml
@@ +392,5 @@
>  
> +            // if tab is a tabbrowser-tab, make sure the browser is instantiated
> +            if (tab.classList.contains("tabbrowser-tab")) {
> +              tab.ensureLinkedBrowser();
> +            }

This needs some documentation to explain why other browsers don't need to be instantiated.
Attachment #8587576 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 118

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #114)

> FF_9_ORIG/Contents/Resources/browser/omni/chrome/browser/content/browser/
> tabbrowser.xml
> @@ +1819,5 @@
> > +            // linkedBrowser.currentURI by the proxy when we don't need to
> > +            // instantiate the browser. This is useful for actions such as
> > +            // opening preferences, which polls every tab for URIs and thus
> > +            // could incidently instantiate the browser for every tab.
> > +            tab.currentURI = Services.io.newURI(aURI,null,null);
> 
> No, that's not a good place to attach it. If the user navigates away and a
> client requests `tab.currentURI`, the client will get the original version
> of `currentURI` which is not the current uri anymore.
> 
> Rather, you should so something along the lines of
>   let originalURI = Services.io.newURI(...);
> 
> // the `get` method of the proxy:
>   `if (key === "currentURI") {
>      return originalURI;
>    }`
> 

I see.  How about simply leveraging the closure and doing away with originalURI:

> // the `get` method of the proxy:
>  if (key === "currentURI") {
>    return Services.io.newURI(aURI,null,null);
>  }


> @@ +1867,5 @@
> > +                tab.linkedBrowser[key] = value;
> > +              }
> > +            });
> > +            
> > +            tab.ensureLinkedBrowser = function() {
> 
> This method needs documentation, especially since it's now public.
>

okay, something like...?

            // provide method to ensure the browser has been instantiated
            tab.ensureLinkedBrowser = function() {
 
> @@ +2737,5 @@
> > +                  if (typeof name === "string") {
> > +                    // test for string because if called
> > +                    // by an iterator, this will croak.
> > +                    let obj = /^(\d+|length)$/.test(name) ? this.tabs : target;
> > +                    return /^\d+$/.test(name) ? obj[name].linkedBrowser : obj[name];
> 
> I think it would be clearer as follows:
> 
> // This property implements an array of `linkedBrowser` for all tabs.
> // However, for performance sake, we try to avoid actually calling
> // `linkedBrowser` until this is necessary.
> if (name == "length") {
>   return this.tabs.length;
> }
> if (Number.isInteger(name)) {
>   return this.tabs[name].linkedBrowser;
> }
> // Anything else is access to a prototype method
> // (e.g. `map`, `filter`, etc.)
> return target[name];
> 

I agree

> @@ +3322,5 @@
> >            this.mCurrentTab.lastAccessed = Infinity;
> >            this.mCurrentTab.linkedBrowser = this.mCurrentBrowser;
> > +          this.mCurrentTab.hasLinkedBrowser = true;
> > +          // we know the browser is linked, so don't do anything
> > +          this.mCurrentTab.ensureLinkedBrowser = function() {};
> 
> Shouldn't we set this as part of `_linkBrowserToTab`?

`addTab` and `_linkBrowserToTab` never get called on the initial tab since the creation of the initial tab and browser is handled differently than all other tabs.  The initial tab and browser are a result of the `tabbrowser` binding, and the code in the constructor is just tying things together.  For simplicity's sake I left things the way they are allowing the initial tab to be instantiated in all cases of start-up (ie not trying to optimize the initial tab), even if it will not be the selected tab.  I just needed to assure that there was an `ensureLinkedBrowser` method attached to the tab so there would not be an error if this method is called on that tab.

In any event, setting the `ensureLinkedBrowser` method `_linkBrowserToTab` would never do, since the method would then never be available until the browser is linked (thus nullifying the point of `ensureLinkedBrowser`)
(Assignee)

Comment 119

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #115)
> Comment on attachment 8587571 [details] [diff] [review]
> SessionStore.jsm optimized to support lazy browser
> 
> Review of attachment 8587571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> FF_9_ORIG/Contents/Resources/browser/omni/modules/sessionstore/SessionStore.
> jsm
> @@ +941,5 @@
> >      }
> >  
> >      var tabbrowser = aWindow.gBrowser;
> >  
> >      // add tab change listeners to all already existing tabs
> 
> (could you add a comment explaining when we'll add the listeners to tabs
> that have not been created or linked yet?)
> 

will do

> @@ +1324,5 @@
> > +      } else {
> > +        delete aTab.__SS_data.pinned;
> > +      }
> > +      aTab.__SS_data.hidden = aTab.hidden;
> > +      let state = JSON.stringify(aTab.__SS_data);
> 
> That looks expensive. Is this something that we already did?
>

Expensive indeed.  I think this would be better, since we should be able to assume a number of conditions being tested for in setTabState are met:

-      let state = JSON.stringify(aTab.__SS_data);
-      this.setTabState(aTab,state);

+      if (aTab.linkedBrowser.__SS_restoreState) {
+        this._resetTabRestoringState(aTab);
+      }
+
+      this.restoreTab(aTab, aTab.__SS_data);

 
> @@ +2415,2 @@
> >            this._resetTabRestoringState(tab);
> > +        }
> 
> Can you document why we don't need to do this for tabs that are not linked
> yet?

will do
(Assignee)

Comment 120

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #117)
> Comment on attachment 8587576 [details] [diff] [review]
> tabbox.xml.diff
> 
> Review of attachment 8587576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> FF_9_ORIG/Contents/Resources/omni/chrome/toolkit/content/global/bindings/
> tabbox.xml
> @@ +392,5 @@
> >  
> > +            // if tab is a tabbrowser-tab, make sure the browser is instantiated
> > +            if (tab.classList.contains("tabbrowser-tab")) {
> > +              tab.ensureLinkedBrowser();
> > +            }
> 
> This needs some documentation to explain why other browsers don't need to be
> instantiated.

will do
(Assignee)

Comment 121

2 years ago
needinfo-ing - David, please respond to comments 118, 119, 120
Flags: needinfo?(dteller)
(Assignee)

Comment 122

2 years ago
> I think it would be clearer as follows:
> 
> ...
>
> if (Number.isInteger(name)) {
>   return this.tabs[name].linkedBrowser;
> }

We need to parseInt since apparently array members are accessed using type `string`, and we still need to test for type `string` since type `symbol` which is passed with `for...of` will throw:

  if (typeof name === "string" && Number.isInteger(parseInt(name))) {
    return this.tabs[name].linkedBrowser;
  }
(Assignee)

Comment 123

2 years ago
(In reply to Allasso Travesser from comment #118)
> I see.  How about simply leveraging the closure and doing away with
> originalURI:
> 
> > // the `get` method of the proxy:
> >  if (key === "currentURI") {
> >    return Services.io.newURI(aURI,null,null);
> >  }

I retract this - we still need to take into account that originalURI will get set to reflect tabData in SessionStore > restoreTab().  I think we want to keep that relationship.  So I agree with your suggestion.
(In reply to Allasso Travesser from comment #118)
> I see.  How about simply leveraging the closure and doing away with
> originalURI:
> 
> > // the `get` method of the proxy:
> >  if (key === "currentURI") {
> >    return Services.io.newURI(aURI,null,null);
> >  }

Well, my version computes the `newURI` only once, while yours computes it several times. Other than that, they are pretty much equivalent, since nsIURI is read-only (iirc).

> > This method needs documentation, especially since it's now public.
> >
> 
> okay, something like...?
> 
>             // provide method to ensure the browser has been instantiated
>             tab.ensureLinkedBrowser = function() {

/**
 * Force full initialization of the browser.
 *
 * In many cases, we initialize the browser lazily, to improve startup speed.
 * Calling this method ensures that the browser is fully initialize. Note,
 * however, that this method is seldom useful for client code, as the browser
 * is able to finish initialization transparently whenever client code attempts
 * to access any of its properties.
 *
 * This method is idempotent.
 */

> > Shouldn't we set this as part of `_linkBrowserToTab`?
> 
> `addTab` and `_linkBrowserToTab` never get called on the initial tab since
> the creation of the initial tab and browser is handled differently than all
> other tabs.  The initial tab and browser are a result of the `tabbrowser`
> binding, and the code in the constructor is just tying things together.  For
> simplicity's sake I left things the way they are allowing the initial tab to
> be instantiated in all cases of start-up (ie not trying to optimize the
> initial tab), even if it will not be the selected tab.  I just needed to
> assure that there was an `ensureLinkedBrowser` method attached to the tab so
> there would not be an error if this method is called on that tab.
> 
> In any event, setting the `ensureLinkedBrowser` method `_linkBrowserToTab`
> would never do, since the method would then never be available until the
> browser is linked (thus nullifying the point of `ensureLinkedBrowser`)

Fair enough, I had misunderstood something in your code. Note, however, that we need to provide a method `ensureLinkedBrowser` (that does nothing) even after the browser has been instantiated.

> Expensive indeed.  I think this would be better, since we should be able to
> assume a number of conditions being tested for in setTabState are met:
> 
> -      let state = JSON.stringify(aTab.__SS_data);
> -      this.setTabState(aTab,state);
> 
> +      if (aTab.linkedBrowser.__SS_restoreState) {
> +        this._resetTabRestoringState(aTab);
> +      }
> +
> +      this.restoreTab(aTab, aTab.__SS_data);

I don't understand this snippet just yet. Can you talk me through it?

> We need to parseInt since apparently array members are accessed using type
> `string`, and we still need to test for type `string` since type `symbol`
> which is passed with `for...of` will throw:
> 
>   if (typeof name === "string" && Number.isInteger(parseInt(name))) {
>     return this.tabs[name].linkedBrowser;
>   }

Ah, right.
Flags: needinfo?(dteller)
(Assignee)

Comment 125

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #124)
> (In reply to Allasso Travesser from comment #118)
>
> ...
> 
> Well, my version computes the `newURI` only once, while yours computes it
> several times. Other than that, they are pretty much equivalent, since
> nsIURI is read-only (iirc).

please see comment 123

> ...Note, however, that
> we need to provide a method `ensureLinkedBrowser` (that does nothing) even
> after the browser has been instantiated.
> 

`ensureLinkedBrowser` does just that:

            tab.ensureLinkedBrowser = function() {
              if (!tab.hasLinkedBrowser) {
                gBrowser._linkBrowserToTab(tab,aURI,options);
              }
            };

tab always has the `ensureLinkedBrowser` method, before and after browser is instantiated.  Note that in the case of the initial tab in the tabbrowser element, the tab never has an un-instantiated state, and so `ensureLinkedBrowser` on that tab just does nothing.

> > -      let state = JSON.stringify(aTab.__SS_data);
> > -      this.setTabState(aTab,state);
> > 
> > +      if (aTab.linkedBrowser.__SS_restoreState) {
> > +        this._resetTabRestoringState(aTab);
> > +      }
> > +
> > +      this.restoreTab(aTab, aTab.__SS_data);
> 
> I don't understand this snippet just yet. Can you talk me through it?
> 

We are removing the `setTabState` call (which would require stringify-ing __SS_data only to be parsed again in setTabState) and calling `restoreTab` directly with the __SS_data object.  There are several other operations done in setTabState, but most of those are only verifying the integrity of the arguments, which shouldn't be needed here.  As far as I can tell, we should only need to `resetTabRestoringState` and `restoreTab`.
Blocks: 1025924
(Assignee)

Comment 126

2 years ago
Created attachment 8592377 [details] [diff] [review]
tabbrowser.xml.diff

tabbrowser methods for optimization - updated code and documentation
Attachment #8592377 - Flags: review?(dteller)
(Assignee)

Comment 127

2 years ago
Created attachment 8592384 [details] [diff] [review]
SessionStore.jsm.diff

SessionStore.jsm code to support lazy browser optimizations
Attachment #8587563 - Attachment is obsolete: true
Attachment #8587571 - Attachment is obsolete: true
Attachment #8587576 - Attachment is obsolete: true
Attachment #8592384 - Flags: review?(dteller)
(Assignee)

Comment 128

2 years ago
Created attachment 8592386 [details] [diff] [review]
tabbox.xml.diff

tabbox.xml - code to support lazy browser optimizations
Attachment #8592386 - Flags: review?(dteller)
Comment on attachment 8592377 [details] [diff] [review]
tabbrowser.xml.diff

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

I think that I'm mostly happy with this. The __SS_data is a bit annoying, of course.

Once you have addressed the comments below, we should find you a peer of browser/ and ask him for an actual review of the patch. Plus it will be good to have a fresh set of eyes on your patch.

::: FF_9_ORIG/Contents/Resources/browser/omni/chrome/browser/content/browser/tabbrowser.xml
@@ +1639,5 @@
> +          <![CDATA[
> +            // bug 906076 - this is called from addTab linkedBrowser proxy,
> +            // to lazily instantiate the browser and related elements, and
> +            // add them to the DOM.
> +            

By the way, don't forget to get rid of these whitespaces.

@@ +1819,5 @@
> +            // linkedBrowser.currentURI by the proxy when we don't need to
> +            // instantiate the browser. This is useful for actions such as
> +            // opening preferences, which polls every tab for URIs and thus
> +            // would incidently instantiate the browser for every tab.
> +            tab.originalURI = Services.io.newURI(aURI,null,null);

Didn't we say that this should be hidden in a closure?

@@ +1845,5 @@
> +                  if (key === "currentURI") {
> +                    return tab.originalURI;
> +                  }
> +                  if (key === "__SS_data" && tab.__SS_data) {
> +                    return tab.__SS_data;

So I take it you haven't managed to get rid of __SS_data in SessionStore.jsm?
At the very least, please document this.

@@ +1883,5 @@
> +            tab.ensureLinkedBrowser = function() {
> +              if (!tab.hasLinkedBrowser) {
> +                gBrowser._linkBrowserToTab(tab,aURI,options);
> +              }
> +            };

Ah, my bad, for some reason, I had assumed that `ensureLinkedBrowser` was a method of the browser itself, which it clearly isn't.
Attachment #8592377 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 130

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #129)
> @@ +1819,5 @@
> > +            // linkedBrowser.currentURI by the proxy when we don't need to
> > +            // instantiate the browser. This is useful for actions such as
> > +            // opening preferences, which polls every tab for URIs and thus
> > +            // would incidently instantiate the browser for every tab.
> > +            tab.originalURI = Services.io.newURI(aURI,null,null);
> 
> Didn't we say that this should be hidden in a closure?

I attempted to clarify this in comment 123, but maybe you missed that.

tab.originalURI can also get overwritten in SessionStore -> restoreTab if doing a "minimal" restore.  So the closure won't work here.
(Assignee)

Updated

2 years ago
Flags: needinfo?(dteller)
But prior to your patch, `tab` didn't have a field called `originalURI`, no?
Flags: needinfo?(dteller)
(Assignee)

Comment 132

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #131)
> But prior to your patch, `tab` didn't have a field called `originalURI`, no?

No it didn't.  `tab.originalURI` was added (originally called `tab.currentURI`) so a valid property could be substituted for `tab.linkedBrowser.currentURI` in order to keep from unnecessarily initiating the browser.  An example of the usefulness of this is when opening Preferences window, or Addon Manager, where `tab.linkedBrowser.currentURI` gets called on every single tab looking to see if the page is already open somewhere, and thus would result in initializing the browser on every tab.
Flags: needinfo?(dteller)
It seems to me that the Proxy (now that you have both `get` and `set`) + closure is sufficient to handle both that case and the fact that SessionRestore can change `currentURI`. What am I missing?
Flags: needinfo?(dteller)
(Assignee)

Comment 134

2 years ago
The closure (the way it was originally suggested) was computing the URI based on aURI passed to (or computed if not present) addTab when creating the tab.  In most cases this would be about:blank.  It does not account for the fact that the tab would be restored to a different URI, which would be more meaningful to pass as a substitute for `tab.linkedBrowser.currentURI`.

I suppose a way to deal with it using the closure would be to derive it from tab.__SS_data within the closure.  But that would seem like we are moving SessionStore code into addTab.  Also extracting the url from __SS_data is a little less than trivial.  In SessionStore this breakdown is already being handled.

Anyway, if you really wanted to use the closure it could look something like:

if (key === "currentURI") {
  let url = tab.__SS_data ? <code to extract url from tab.__SS_data> ? aURI
  return Services.io.newURI(url,null,null);
}
Flags: needinfo?(dteller)
(Assignee)

Comment 135

2 years ago
Truly the URI extracted from the url obtained __SS_data is more meaningful, because this is most closely what `tab.linkedBrowser.currentURI` will be once the browser is initialized.
No, let's not extract stuff from __SS_data.

I believe that we have two scenarios.

1/ Before we have changed `browser.currentURI`:

if (key == "currentURI") {
  return originalURI; // returns the correct uri
}

2/ Once we have changed `browser.currentURI`:

Doing `browser.currentURI` (in Session Restore?) will call the Proxy's `set`, which calls `_linkBrowserToTab` and then `tab.linkedBrowser[key] = newURI`, so calling `browser.currentURI` will return `newURI`.

So what am I missing?
Flags: needinfo?(dteller)
Comment on attachment 8592384 [details] [diff] [review]
SessionStore.jsm.diff

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

::: FF_9_ORIG/Contents/Resources/browser/omni/modules/sessionstore/SessionStore.jsm
@@ +954,1 @@
>      }

Since we are not using `i`, could you take the opportunity to rewrite that loop as

for (let tab of tabbrowser.tabs) {
  // ...
}

?

@@ +1307,5 @@
>      }
>    },
>  
>    /**
> +   * bug 906076:

Generally, you don't need to specify the bug# in comments. `hg blame` does it for you.

@@ +2414,5 @@
>        tabbrowser.showTab(tabs[0]);
>      }
>  
> +    // If overwriting tabs ("overwrite" meaning, we will be restoring a tab to
> +    // a new state that has previously been restored) we want to reset each

// If overwriting tabs (i.e. restoring a tab that has already been restored to a new state), we want to reset each tab's  ...

@@ +2420,5 @@
> +    // no longer be restoring. The tabs will be rebuilt and marked if they need
> +    // to be restored after loading state (in restoreTabs). (Note that this only
> +    // has relavence for a mid-session restore, for on startup we are not
> +    // overwriting tabs which have previously been restored.)
> +    // We ignore tabs where the browser has not been initialized (bug 906076)

Also, in your comments, you use sometimes "link", "initialize", "instantiate", please pick only one of these words and use it everywhere.

@@ +2678,3 @@
>                                              {tabData: tabData, epoch: epoch});
> +    } else {
> +      tab.__SS_data = tabData;

You are putting things in `__SS_data` that we didn't use to put here. Can you convince me that we're not conflicting with the existing uses of `__SS_data`?

@@ +2680,5 @@
> +      tab.__SS_data = tabData;
> +      let entries = tabData.entries;
> +      let title;
> +      let url;
> +      let image;

Nit: Please move `url` and `image` inside the `if`.

@@ +2688,5 @@
> +        image = tabData.image;
> +        if (image) {
> +          tab.setAttribute("image",image);
> +        }
> +        else {

Nit: } else {

@@ +2696,5 @@
> +        // linkedBrowser.currentURI by the proxy when we don't need to
> +        // instantiate the browser. This is useful for actions such as opening
> +        // preferences, which polls every tab for URIs and thus could
> +        // instantiate the browser on every tab.
> +        tab.originalURI = Services.io.newURI(url,null,null);

(pending discussion on `originalURI`)
Attachment #8592384 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 138

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #137)
> @@ +2678,3 @@
> >                                              {tabData: tabData, epoch: epoch});
> > +    } else {
> > +      tab.__SS_data = tabData;
> 
> You are putting things in `__SS_data` that we didn't use to put here. Can
> you convince me that we're not conflicting with the existing uses of
> `__SS_data`?

We are just setting a reference to tabData in restoreTab, to hold it until it is needed to complete the restore on browser instantiation.  The only place any of the members of __SS_data are altered are in ssi_onTabLinkBrowser just after instantiation and just before full restore.  The `hidden` and `pinned` properties are updated to reflect their current state, as it is possible those states may now actually be different than what the corresponding properties in __SS_data reflect.  This must be done because restoreTab will show/hide/pin/unpin the tab based on that data and we don't want it to alter its current state.
(Assignee)

Updated

2 years ago
Flags: needinfo?(dteller)
(Assignee)

Comment 139

2 years ago
Created attachment 8593674 [details] [diff] [review]
tabbrowser.xml.diff

tabbrowser.xml - updated patch - clarify comments
Attachment #8592377 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8593674 - Flags: ui-review?
Attachment #8593674 - Flags: review?(dteller)
(Assignee)

Comment 140

2 years ago
Created attachment 8593675 [details] [diff] [review]
SessionStore.jsm.diff

SessionStore.jsm - updated patch - clarified comments, cleaned up code per David's request
Attachment #8593675 - Flags: review?(dteller)
(Assignee)

Comment 141

2 years ago
Created attachment 8593676 [details] [diff] [review]
tabbox.xml.diff

tabbox.xml - clarified code
Attachment #8592384 - Attachment is obsolete: true
Attachment #8592386 - Attachment is obsolete: true
Attachment #8592386 - Flags: review?(dteller)
Attachment #8593676 - Flags: review?(dteller)
(Assignee)

Updated

2 years ago
Attachment #8593674 - Flags: ui-review? → ui-review-
(Assignee)

Comment 142

2 years ago
Hi, David, I have addressed all of your requests for code changes/comment clarification in comment 137 and uploaded a new set of patches.

I replied to your questions regarding __SS_data in comment 138.
Flags: needinfo?(dteller)
The __SS_data stuff is still a bit too scary for me. At the moment, according to https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=__SS_Data&redirect=true, __SS_data is used in 17 places in the source code of Firefox (not counting tests, Metro or Android). Any interaction with __SS_data will add to the existing mess.

I believe that the right way to do this is to first fix unmessify a bit the use of __SS_data (in a different bug that will block this one) as follows:
- introduce a new API that replaces all the uses of __SS_data outside of SesisonStore.jsm – let's call this `SessionStore.getDelayedTabRestoreData(tab)`;
- replace all calls to `browser.__SS_data` (both in SessionStore.jsm and in client code) with calls to `getDelayedTabRestoreData`;
- internally, this API as a WeakMap from tab to object (see DyingWindowCache in SessionStore.jsm as an example).

This will have the following benefits:
- code that is a little less messy;
- no need for introducing a special case for __SS_data in tabbrowser.xml;
- an opportunity to document what's in __SS_data in the first place.

Does this sound ok to you?
Flags: needinfo?(dteller)
(Assignee)

Comment 144

2 years ago
Seems good to get it cleaned up and keep everything inside SessionStore.  Do you want me to work on the patch?  I'm happy to do it, though I may be asking questions.  Do you want to start the bug or shall I?  I'm not sure how to describe it.

Note that as far as I can tell, we'll still need to update the object's hidden/pinned properties in onTabLinkBrowser to keep things in sync when doing restoreTab on a mid-session tab whose browser is newly instantiated.  I suppose an alternative way of dealing with this would be to add a parameter/flag to restoreTab which will skip setting tab's hidden and pinned states, but that seems kinda ugly.

I only see two other places where __SS_data properties are modified:

	browser/metro/components/SessionStore.js
397 	browser.__SS_data.formdata = aMessage.json.data;
400 	browser.__SS_data.scroll = aMessage.json.data;
Depends on: 867118
I was about to open the bug and then found out bug 867118 which is exactly about getting rid of __SS_data, complete with a patch (that apparently doesn't fully work, you'll have to find out why).

Regarding browser/metro, the project has been cancelled, so I wouldn't worry too much about it.
(Assignee)

Comment 146

2 years ago
So how should we deal with the issue of not changing state for a mid-session tab (full) restore based on stale tabData/__SS_data? (ie, pinned/hidden states).

Right now (ignoring browser/metro) __SS_data is being used read-only.  Is that a status we'd like to maintain, or does it matter?  Maybe one thing we might consider: Is the role of __SS_data (whatever we'll call it) changing now because it is being kept around longer?
Flags: needinfo?(dteller)
Comment on attachment 8593674 [details] [diff] [review]
tabbrowser.xml.diff

Canceling review until we have dealt with `__SS_data`.
Also, canceling ui-review, which looks like it was set by error.
Flags: needinfo?(dteller)
Attachment #8593674 - Flags: ui-review-
Attachment #8593674 - Flags: review?(dteller)
Attachment #8593675 - Flags: review?(dteller)
Attachment #8593676 - Flags: review?(dteller)
(In reply to Allasso Travesser from comment #146)
> So how should we deal with the issue of not changing state for a mid-session
> tab (full) restore based on stale tabData/__SS_data? (ie, pinned/hidden
> states).

I'm not sure I understand. Could you give me an example?

> Right now (ignoring browser/metro) __SS_data is being used read-only.  Is
> that a status we'd like to maintain, or does it matter?  Maybe one thing we
> might consider: Is the role of __SS_data (whatever we'll call it) changing
> now because it is being kept around longer?

If we can keep __SS_data read-only, that would be great.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 149

2 years ago
Right now, in restoreTab, we get the hidden and pinned properties from tabData (__SS_data == tabData) and by these properties set the hidden and pinned states of the tab.  This has worked well because we are normally restoring tabs forthright on a window restore:

    // Prepare the tab so that it can be properly restored. We'll pin/unpin
    // and show/hide tabs as necessary. We'll also attach a copy of the tab's
    // data in case we close it before it's been restored.
    if (tabData.pinned) {
      tabbrowser.pinTab(tab);
    } else {
      tabbrowser.unpinTab(tab);
    }

    if (tabData.hidden) {
      tabbrowser.hideTab(tab);
    } else {
      tabbrowser.showTab(tab);
    }

But now we have a situation where the tab may be "restored" twice - the first time a minimal restore where we don't want to instantiate the browser, the second time where we are actually ready to use the tab and it gets fully restored.  We are restoring the second time from the same tabData which was used to restore the first time (it has been carried around in __SS_data, or however we decide to do it once bug 867118 is finished).  However, in the meanwhile the tab's pinned or hidden state may have been changed (user switched groups, or user pinned/unpinned the tab).

In the submitted patches this has been handled by updating the pinned and hidden properties of __SS_data before passing them to restoreTab.

However we choose to do it, we need a way of assuring that when a tab undergoes a later full restore, its hidden and pinned states don't get altered.
Flags: needinfo?(allassopraise) → needinfo?(dteller)
(Assignee)

Comment 150

2 years ago
Note also, that while at first glance it may seem a simple solution to simply not set pinned/hidden states on a full restore, this will not work because in some cases (selected tab) we will be doing a full restore forthright during restoreWindow, in which case the pinned/hidden states need to be set.
Ok, let's update the pinned and hidden properties of whatever replaces __SS_data. If we find a better way at a later stage, we can always update the code.
Flags: needinfo?(dteller)
(Assignee)

Comment 152

2 years ago
Okay.  Thank you.
(Assignee)

Comment 153

2 years ago
With the resolution of bug 1166757 (which I think is a good thing to get that cleaned up), for purposes of lazy browser tabs it would be good to use xul:tab as key instead of xul:browser for TabStateCache.  In general, I am seeing that if lazy browser is to become a reality, then we should be shifting our view to referencing everything (as much as possible) to the tab rather than the browser.  This would make much more sense to me.  The patches that have been made on this bug so far have been kinda trying to play both sides of the fence, and that has made things awkward and clumsy.  Perhaps we could also reference permanentKey to the tab rather than the browser as well, though I haven't looked into that in depth to see if that is feasible.

I would like to suggest to start off with that we change the key for TabStateCache to xul:tab, which would immediately deal with a lot of awkwardness.  Whether this change should be included in this bug, or start a bug of its own, someone please advise.

The concept of lazy browser tabs is somewhat invasive indeed and it does require a paradigm shift.  But I believe that if we can start laying some groundwork for it now it could go pretty smoothly and not be messy.
Flags: needinfo?(ttaubert)
Flags: needinfo?(dteller)
(Assignee)

Comment 154

2 years ago
All of the above of course is with consideration of the impact on addons.  The point is that I think we can look for ways to lay some foundation to bring this in smoothly, looking for things that just do their work quietly in the background.  I think changing keys to xul:tab in TabStateCache is a good example of a low-impact change that would yield a lot in the way of moving to lazy browser tabs.
Depends on: 1167923
> I would like to suggest to start off with that we change the key for TabStateCache to xul:tab, which would immediately deal with a lot of awkwardness.  Whether this change should be included in this bug, or start a bug of its own, someone please advise.

Agreed. Filed as bug 1167923. It's all yours :)
Flags: needinfo?(dteller)
(Assignee)

Comment 156

2 years ago
David, should I diff against bug 1166757 patch?
Flags: needinfo?(dteller)
(Assignee)

Comment 157

2 years ago
never mind.
Flags: needinfo?(dteller)
Flags: needinfo?(ttaubert)

Comment 158

2 years ago
What's the next step for this bug?
Allasso, I guess that we are ready to proceed. I seem to remember that you can publish a patch that doesn't need to touch SessionRestore now, right?
(Assignee)

Comment 160

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (away June 22 - July 7th, use "needinfo") from comment #159)
> ...I seem to remember that you
> can publish a patch that doesn't need to touch SessionRestore now, right?

I am not sure about that one, I'll re-examine the code to see if the shared TabStateCache key gets us completely free of that.
(Assignee)

Comment 161

2 years ago
Created attachment 8632252 [details] [diff] [review]
PH 7 patch - implementation and handling of lazy-browser tabs

Next phase of this bug.  Quite a few differences from previous patch.

Incorporates code changes to harmonize with/leverage bug 1166757 and bug 1167923.

Also changes necessary due to various other bugs that have landed since then in tabbrowser.xml and TabState.jsm.

I have put some comments in the code which beg advisement.

David, please look closely at the permanentKey sharing strategy.  It is a bit kludgy and I would appreciate any suggestions.  The challenge here is that a preload browser can exist while an uninstantiated tab exists, and both need a permanentKey before linking.  Since linking is async now, there is no way of knowing ahead of time which preload browser will get linked to which tab.

Also wondering about breaking up SS > restoreTab.  Rather than putting all the conditional code in restoreTab, maybe have, eg, `preRestoreTab`, where the conditionals would call either, say, `fullyRestoreTab` or `lightRestoreTab`.

Also wondering if this would be better in tabbox.xml:
Rather than testing to see if the tab is a tabbrowser tab and calling tab.ensureLinkedBrowser directly, to send an event, eg, `TabSelected` and deal with it in a handler in either tabbrowser or SessionStore.  It seems out of place to deal with stuff specific to tabbrowser tabs in generic tabs code.

Sorry, but it doesn't seem like there is ever going to be any way around patching SessionStore.
Attachment #8587574 - Attachment is obsolete: true
Attachment #8593674 - Attachment is obsolete: true
Attachment #8593675 - Attachment is obsolete: true
Attachment #8593676 - Attachment is obsolete: true
Attachment #8632252 - Flags: feedback?(dteller)
Using mozReview would be really much more comfortable. You need a L1 agreement to do that. Could you request one? I can vouch for you.

Instructions are here: https://www.mozilla.org/hacking/committer/
Flags: needinfo?(allassopraise)
(Assignee)

Comment 163

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #162)
> Using mozReview would be really much more comfortable. You need a L1
> agreement to do that. Could you request one? I can vouch for you.
> 
> Instructions are here: https://www.mozilla.org/hacking/committer/

here you go:

https://bugzilla.mozilla.org/show_bug.cgi?id=1183165
Flags: needinfo?(allassopraise)
Comment on attachment 8632252 [details] [diff] [review]
PH 7 patch - implementation and handling of lazy-browser tabs

I'll wait for the ReviewBoard version.
Attachment #8632252 - Flags: feedback?(dteller)
(Assignee)

Comment 165

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #164)
> Comment on attachment 8632252 [details] [diff] [review]
> PH 7 patch - implementation and handling of lazy-browser tabs
> 
> I'll wait for the ReviewBoard version.

I've had a rough time trying to get this thing going, I'm hung up on trying to push changes to reviewboard, one obstacle after the next.  I've spent too much time on it already, and will have to work on it more when I have time to.
Sorry about that. I realize that it's yet another obstacle in your road, but in my experience, it really makes reviews easier.

For me, the steps were:

1/ Grab the latest version of mozilla-central;
2/ Run `./mach mercurial-setup` (and accept at least the reviewboard stuff);
3/ `hg push review`;
4/ That's it.
(Assignee)

Comment 167

2 years ago
Created attachment 8634113 [details]
MozReview Request: bug 906076 - create linkedBrowser lazily for tabbrowser tabs for performance optimization r?dteller@mozilla.com

bug 906076 - create linkedBrowser lazily for tabbrowser tabs for performance optimization r?dteller@mozilla.com
Attachment #8634113 - Flags: review?(dteller)
Attachment #8634113 - Flags: review?(dteller)
Comment on attachment 8634113 [details]
MozReview Request: bug 906076 - create linkedBrowser lazily for tabbrowser tabs for performance optimization r?dteller@mozilla.com

https://reviewboard.mozilla.org/r/13319/#review11993

N

::: browser/base/content/tabbrowser.xml:1658
(Diff revision 1)
> -            b.permanentKey = {};
> +            b.permanentKey = aParams && aParams.permanentKey ||

Clearly, ttaubert understands the permanent key better than I do, so once we are done, we'll need to ask him for a further review.

::: browser/base/content/tabbrowser.xml:1724
(Diff revision 1)
> +            // bug 906076 - this is called from addTab linkedBrowser proxy,

Nit: The bug number is not useful here.

::: browser/base/content/tabbrowser.xml:1769
(Diff revision 1)
> +            aTab.hasLinkedBrowser = true;

Nit: I don't think that you have documented `hasLinkedBrowser`. You should document it somewhere (probably not here, though).

::: browser/base/content/tabbrowser.xml:1787
(Diff revision 1)
> +            let position = aTab._tPos;

Mmmh... That line seems new, right? Could you explain it to me?

I _think_ I understand `position`, but it might deserve some doc.

::: browser/base/content/tabbrowser.xml:1772
(Diff revision 1)
> +            if (!notificationbox.parentNode) {

As a side-note, it looks like this bit can be delayed. But that's food for a followup bug.

::: browser/base/content/tabbrowser.xml:1810
(Diff revision 1)
> +                { browser.userTypedValue = aURI; }

Nit: I believe that you have added the braces, so can you make sure that they look like the rest of the code? i.e.

```
if (foo) {
  // ...
}
```

Here and in a few other places below.

::: browser/base/content/tabbrowser.xml:1830
(Diff revision 1)
> +                Cu.reportError(ex);

Note for a followup bug: we should improve error-reporting.

::: browser/base/content/tabbrowser.xml:1899
(Diff revision 1)
> -            // the uri to load can be loaded remotely.
> +            tab.setAttribute("onerror", "this.removeAttribute('image');");

Ouch. That's not your fault, but this is ugly :)

::: browser/base/content/tabbrowser.xml:1863
(Diff revision 1)
>              const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

Could you take the opportunity to add `"use strict";` at the start of all your methods? I'm not 100% sure this works in XBL, but if it does, this will help us catch some errors.

::: browser/base/content/tabbrowser.xml:1927
(Diff revision 1)
> -            let b;
> +            // compute URI object from aURI and set it to the tab, which can be

Nit: This documentation is useful, but a bit hard to understand.

::: browser/base/content/tabbrowser.xml:1946
(Diff revision 1)
> -            // If we open a new tab with the newtab URL,
> +            // bug 906076 - We use a proxy to lazily instantiate the browser

Nit: No need to add the bug#. Other than that, the comment is good.

::: browser/base/content/tabbrowser.xml:1954
(Diff revision 1)
> +            tab.linkedBrowser = new Proxy({},{

Nit: Whitespace after `,`.

::: browser/base/content/tabbrowser.xml:1960
(Diff revision 1)
> +                  if (key === "userTypedValue") {

Shouldn't `userTypedValue` be `aURI`?

::: browser/base/content/tabbrowser.xml:1960
(Diff revision 1)
> +                  if (key === "userTypedValue") {

Shouldn't this be `aURI`?

::: browser/base/content/tabbrowser.xml:1964
(Diff revision 1)
> +                    return null;

I believe that `userTypedClear` is a number, in which case I think it should be `0`.

::: browser/base/content/tabbrowser.xml:1970
(Diff revision 1)
> +                if (key in browser) {

I'm not sure we need to check if `key in browser`. Wouldn't it work regardless?

::: browser/base/content/tabbrowser.xml:2000
(Diff revision 1)
> +                gBrowser._linkBrowserToTab(tab,aURI,tab.permanentKey,options);

Nit: space after commas.

::: browser/base/content/tabbrowser.xml:2005
(Diff revision 1)
> -            var position = this.tabs.length - 1;
> +            tab._tPos = position;

Could you take the opportunity to document `_tPos`?

::: browser/base/content/tabbrowser.xml:2006
(Diff revision 1)
> -            var uniqueId = this._generateUniquePanelID();
> +            tab.lastAccessed = Date.now();

I'd like Tim to double-check that `lastAccessed` is correct.

::: browser/base/content/tabbrowser.xml:2056
(Diff revision 1)
> +              tab.ensureLinkedBrowser();

Just be sure: have you checked that Session Restore doesn't provide the URI for tabs that are restored lazily? If so, a comment could be useful.

::: browser/base/content/tabbrowser.xml:4003
(Diff revision 1)
> +          // what would be the consequences to all concerned of returning false

I don't think it is a good idea to toy with `isRemoteBrowser` in general, so I believe that your change is sufficient.

::: browser/base/content/tabbrowser.xml:4155
(Diff revision 1)
> +          this.mCurrentTab.permanentKey = this.mCurrentBrowser.permanentKey;

Here, too, I'd like ttaubert's input.

::: browser/components/sessionstore/SessionStore.jsm:795
(Diff revision 1)
> -        this.onTabAdd(win, target);
> +        this.onTabLinkBrowser(win, aEvent.originalTarget);

Nit: Let's keep `target` here, for consistency.

::: browser/components/sessionstore/SessionStore.jsm:882
(Diff revision 1)
> -    for (let i = 0; i < tabbrowser.tabs.length; i++) {
> +    // For tabs which have not yet had their browser instantiated (bug 906076),

Good comment. No need to add the bug#, though.

I like what I see, with a few minor questions/nits above.

Now, you will need to write a test. This may be tricky, but I suggest we make it a new Session Restore test that does the following:
- create (in memory) a sessionstore.js and use it for restoring (in the Session Restore test suite, this is implemented by function `promiseBrowserState`);
- check that all the tabs except the current tab has `hasLinkedBrowser` set to `false`.

What do you think?
I'm not entirely done reviewing, btw. I'll try and finish later today.

Also, have you run the unit tests yet?
Flags: needinfo?(allassopraise)
Assignee: nobody → allassopraise
(Assignee)

Comment 170

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #169)
> Also, have you run the unit tests yet?

I don't know what that is, can you point me to some documentation?
Flags: needinfo?(allassopraise)
https://reviewboard.mozilla.org/r/13319/#review12053

::: browser/components/sessionstore/SessionStore.jsm:1407
(Diff revision 1)
> +    // for now it looks safe to just return if the tab has been restored,

Get rid of the long comment and just:
```
if (browser.__SS_restoreState) {
  // The tab has already been restored.
  return;
}
```

How can we end up in this situation, though?

::: browser/components/sessionstore/SessionStore.jsm:1418
(Diff revision 1)
> +    // simply setting aTab.permanentKey = browser.permanentKey appears like it

Note: In the future, if it's a question to the reviewer, can you make this clear in the comment?

For this as for other permanent key-related stuff, I'd like tim's feedback.

::: browser/components/sessionstore/SessionStore.jsm:2846
(Diff revision 1)
> +    // accessing linkedBrowser properties will force instantiation of the

Nit: Please start your sentences with an uppercase letter.

::: browser/components/sessionstore/SessionStore.jsm:1427
(Diff revision 1)
> +    let tabState = TabState.clone(aTab);

I'm not sure I get it.

`TabState.clone` (or `TabState.collect`) is going to extract the current state from the tab. Since you are in the process of restoring the tab, how can this work?

::: browser/components/sessionstore/SessionStore.jsm:1428
(Diff revision 1)
> +    this.restoreTab(aTab, tabState);

I believe that the call to `restoreTab` makes sense, but could you document it?

::: toolkit/content/widgets/tabbox.xml:411
(Diff revision 1)
> +            if (tab.classList.contains("tabbrowser-tab")) {

Instead of relying on the `classList`, I would check for the presence of `ensureLinkedBrowser`:

```
if ("ensureLinkedBrowser" in tab) {
  tab.ensureLinkedBrowser();
}
```

::: toolkit/content/widgets/tabbox.xml:397
(Diff revision 1)
> +            /**

Nit: `/**` is for documentation. Here, `/*` or `//` are more appropriate.
(Assignee)

Comment 172

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #171)
> https://reviewboard.mozilla.org/r/13319/#review12053
> 
> ::: browser/components/sessionstore/SessionStore.jsm:1407
> (Diff revision 1)
> > +    // for now it looks safe to just return if the tab has been restored,
> 
> Get rid of the long comment and just:
> ```
> if (browser.__SS_restoreState) {
>   // The tab has already been restored.
>   return;
> }
> ```
> 
> How can we end up in this situation, though?

It is possible that `onTabLinkBrowser` can get called on a tab after it has been restored:

The initial tab in the `tabbrowser` binding gets the browser linked in the constructor and will get restored during a window restore via restoreTabs.  But then it is possible for `onTabLinkBrowser` to get called explicitly on that tab in the window's onload handler in SessionStore, afterwards.

> For this as for other permanent key-related stuff, I'd like tim's feedback.

I really hate all of this, it is really ugly.  The preloaded browser is kind of a fly in the ointment to this bug.  I believe the whole purpose of preloaded browser is to get rid of the flicker what would occur when opening a new tab when waiting for about:blank to be replaced with about:newtab (I can't remember the bug number).  I wonder if there is another way to accomplish this?  I suppose though that preloaded browser is pretty embedded.  Have any thoughts on this?

> ::: browser/components/sessionstore/SessionStore.jsm:1427
> (Diff revision 1)
> > +    let tabState = TabState.clone(aTab);
> 
> I'm not sure I get it.
> 
> `TabState.clone` (or `TabState.collect`) is going to extract the current
> state from the tab. Since you are in the process of restoring the tab, how
> can this work?

`TabState.clone` or `TabState.collect` access (through _collectBaseTabData > copyFromCache) `TabStateCache` to get most of its data.  The exceptions are, for the most part, stuff that is set on the minimal tab, which we want to get the current state anyway: `tab.pinned`, `tab.hidden`, tab's attributes, icon.  The only exception to this (as far as I can see) are `userTypedValue` and `userTypedClear`, which is being trapped in the proxy.

The alternative would be to call `TabState.copyFromCache` instead, and set the properties and attribute's explicitly.  This would negate the need for trapping `userTypedValue` and `userTypedClear` in the proxy.

I just used `TabState.clone` because it "seemed" cleaner and seems to work.

Any thoughts on that?

> Instead of relying on the `classList`, I would check for the presence of
> `ensureLinkedBrowser`:
> 
> ```
> if ("ensureLinkedBrowser" in tab) {
>   tab.ensureLinkedBrowser();
> }
> ```

Do you have any thoughts on dispatching an event instead, as described in comment 161:

> Also wondering if this would be better in tabbox.xml:
> Rather than testing to see if the tab is a tabbrowser tab and calling
> tab.ensureLinkedBrowser directly, to send an event, eg, `TabSelected`
> and deal with it in a handler in either tabbrowser or SessionStore.
> It seems out of place to deal with stuff specific to tabbrowser tabs
> in generic tabs code.
Flags: needinfo?(dteller)
(Assignee)

Comment 173

2 years ago
https://reviewboard.mozilla.org/r/13319/#review12053

> Instead of relying on the `classList`, I would check for the presence of `ensureLinkedBrowser`:
> 
> ```
> if ("ensureLinkedBrowser" in tab) {
>   tab.ensureLinkedBrowser();
> }
> ```

Do you have any thoughts on dispatching an event instead:

Rather than testing to see if the tab is a tabbrowser tab and calling tab.ensureLinkedBrowser directly, to send an event, eg, `TabSelected` and deal with it in a handler in either tabbrowser or SessionStore.  It seems out of place to deal with stuff specific to tabbrowser tabs in generic tabs code.

> I'm not sure I get it.
> 
> `TabState.clone` (or `TabState.collect`) is going to extract the current state from the tab. Since you are in the process of restoring the tab, how can this work?

`TabState.clone` or `TabState.collect` access `TabStateCache` to get most of its data.  The exceptions are, for the most part, stuff that is set on the minimal tab, which we want to get the current state anyway: `tab.pinned`, `tab.hidden`, tab's attributes, icon.  The only exception to this (as far as I can see) are `userTypedValue` and `userTypedClear`, which is being trapped in the proxy.

The alternative would be to call `TabState.copyFromCache` instead, and set the properties and attribute's explicitly.  This would negate the need for trapping `userTypedValue` and `userTypedClear` in the proxy.

I just used `TabState.clone` because it "seemed" cleaner and seems to work.

Any thoughts on that?

> Note: In the future, if it's a question to the reviewer, can you make this clear in the comment?
> 
> For this as for other permanent key-related stuff, I'd like tim's feedback.

I really hate all of this, it is really ugly.  The preloaded browser is kind of a fly in the ointment to this bug.  I believe the whole purpose of preloaded browser is to get rid of the flicker what would occur when opening a new tab when waiting for about:blank to be replaced with about:newtab (I can't remember the bug number).  I wonder if there is another way to accomplish this?  I suppose though that preloaded browser is pretty embedded.  Have any thoughts on this?

> Get rid of the long comment and just:
> ```
> if (browser.__SS_restoreState) {
>   // The tab has already been restored.
>   return;
> }
> ```
> 
> How can we end up in this situation, though?

It is possible that `onTabLinkBrowser` can get called on a tab after it has been restored:

The initial tab in the `tabbrowser` binding gets the browser linked in the constructor and will get restored during a window restore via restoreTabs.  But then it is possible for `onTabLinkBrowser` to get called explicitly on that tab in the window's onload handler in SessionStore, afterwards.
(Assignee)

Comment 174

2 years ago
sorry for the redundant posting - getting used to reviewboard :-)
(Assignee)

Comment 175

2 years ago
https://reviewboard.mozilla.org/r/13319/#review11993

> Mmmh... That line seems new, right? Could you explain it to me?
> 
> I _think_ I understand `position`, but it might deserve some doc.

In the original code, position was taken as the last tab in the list since a new tab was just created and placed there.  With lazy browser instantiation, the code for instantiating the browser has been split off from `addTab`, and so when we finally instantiate the browser we have to get the position from where the tab currently is, which could be anyway at that point.

> Shouldn't this be `aURI`?

Yes, that would make more sense.

> I'm not sure we need to check if `key in browser`. Wouldn't it work regardless?

Well, I suppose if someone calls `tab.linkedBrowser.doodlebug` it would just return undefined.  Checking `key in browser` seemed a little more proper to me, but then I guess if there is no `else { return undefined; }` then it is not so proper after all.  If you think the test should be removed, I'll remove it.

> Just be sure: have you checked that Session Restore doesn't provide the URI for tabs that are restored lazily? If so, a comment could be useful.

I'm not clear on what you are asking here.

> Nit: Let's keep `target` here, for consistency.

hmmm, not sure why I did that.  Okay.

Can you point me to some help on writing tests, so I am sure I'm on the right track?

ps, you're right, reviewboard is much better.
(Assignee)

Comment 176

2 years ago
https://reviewboard.mozilla.org/r/13319/#review12053

> I really hate all of this, it is really ugly.  The preloaded browser is kind of a fly in the ointment to this bug.  I believe the whole purpose of preloaded browser is to get rid of the flicker what would occur when opening a new tab when waiting for about:blank to be replaced with about:newtab (I can't remember the bug number).  I wonder if there is another way to accomplish this?  I suppose though that preloaded browser is pretty embedded.  Have any thoughts on this?

Well, I had thoughts about just swapping doc shells from the preload browser instead of linking the entire browser, but bug 1077652 shoots that idea down :-\.  I don't get why we are needing to track stuff, or at least track tab state for a preload browser though (using permanentKey).  Isn't it just a browser waiting in the background to be used when it is needed?  (BTW, the preload idea was first introduced in bug 753448.)
https://reviewboard.mozilla.org/r/13319/#review11993

> In the original code, position was taken as the last tab in the list since a new tab was just created and placed there.  With lazy browser instantiation, the code for instantiating the browser has been split off from `addTab`, and so when we finally instantiate the browser we have to get the position from where the tab currently is, which could be anyway at that point.

Still, can you add doc to the source code?

> I'm not clear on what you are asking here.

If `aURI` is defined (and is not "about:blank"), the browser is instantiated immediately. During startup, if we open 300 tabs, is `aURI` defined at this line?

> Well, I suppose if someone calls `tab.linkedBrowser.doodlebug` it would just return undefined.  Checking `key in browser` seemed a little more proper to me, but then I guess if there is no `else { return undefined; }` then it is not so proper after all.  If you think the test should be removed, I'll remove it.

I think that removing it makes the code slightly more readable.
https://reviewboard.mozilla.org/r/13319/#review12053

> Well, I had thoughts about just swapping doc shells from the preload browser instead of linking the entire browser, but bug 1077652 shoots that idea down :-\.  I don't get why we are needing to track stuff, or at least track tab state for a preload browser though (using permanentKey).  Isn't it just a browser waiting in the background to be used when it is needed?  (BTW, the preload idea was first introduced in bug 753448.)

Tim knows the preloaded browser much better than me, so you should ask him. If you wish a synchronous conversation, he's often on irc, channel fx-team. Expect European times.

> `TabState.clone` or `TabState.collect` access `TabStateCache` to get most of its data.  The exceptions are, for the most part, stuff that is set on the minimal tab, which we want to get the current state anyway: `tab.pinned`, `tab.hidden`, tab's attributes, icon.  The only exception to this (as far as I can see) are `userTypedValue` and `userTypedClear`, which is being trapped in the proxy.
> 
> The alternative would be to call `TabState.copyFromCache` instead, and set the properties and attribute's explicitly.  This would negate the need for trapping `userTypedValue` and `userTypedClear` in the proxy.
> 
> I just used `TabState.clone` because it "seemed" cleaner and seems to work.
> 
> Any thoughts on that?

I'd go for `copyFromCache`.

> Do you have any thoughts on dispatching an event instead:
> 
> Rather than testing to see if the tab is a tabbrowser tab and calling tab.ensureLinkedBrowser directly, to send an event, eg, `TabSelected` and deal with it in a handler in either tabbrowser or SessionStore.  It seems out of place to deal with stuff specific to tabbrowser tabs in generic tabs code.

Yes, I believe that this makes sense. Session Restore uses "TabShow" for this purpose, would that fit your needs?
https://reviewboard.mozilla.org/r/13319/#review11993

The documentation on tests is not very useful, unfortunately. You can find some here: https://developer.mozilla.org/en-US/docs/Mochitest.

Generally, you should look at the tests in browser/components/sessionstore/test/browser and work from the examples. You'll need to name your new file `browser_somethingSomething.js`, add it to `browser.ini`, and use `./mach mochitest path/to/your/file` to test it.

You could draw some inspiration from `browser_cleaner.js`, to get started.
(Assignee)

Comment 180

2 years ago
https://reviewboard.mozilla.org/r/13319/#review11993

> If `aURI` is defined (and is not "about:blank"), the browser is instantiated immediately. During startup, if we open 300 tabs, is `aURI` defined at this line?

Oh, yes, I see now.  Yes, code for restore is: `tabbrowser.addTab("about:blank", {skipAnimation: true}));`

okay, thanks.
(Assignee)

Comment 181

2 years ago
https://reviewboard.mozilla.org/r/13319/#review12053

> Yes, I believe that this makes sense. Session Restore uses "TabShow" for this purpose, would that fit your needs?

The `TabShow` event is fired when a tab's hidden state is changed; afaik this is only ever initiated by TabView when changing groups.  I am not aware of an event that gets fired when tab selection actually occurs.  Listening for `TabSelect` (which gets fired for tabbrowser tabs in `updateCurrentBrowser`) won't do because there are cases when tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).  So afaict, we'll need to create a new event (TabSelected? or TabIndexSelected?).  Should this be documented as being for lazy-browser tabs?  I am wondering that because it is a generic event which in theory could be used by something else.

So do you believe it would be better to have the handler in SessionStore rather than tabbrowser?

> Tim knows the preloaded browser much better than me, so you should ask him. If you wish a synchronous conversation, he's often on irc, channel fx-team. Expect European times.

Okay.  I have started researching preloaded browser and would like to continue that first, in order to have a more meaningful dialog.  I may be able to just answer the question myself.  iae, I hope to find *some* way make this cleaner.
https://reviewboard.mozilla.org/r/13319/#review12053

> It is possible that `onTabLinkBrowser` can get called on a tab after it has been restored:
> 
> The initial tab in the `tabbrowser` binding gets the browser linked in the constructor and will get restored during a window restore via restoreTabs.  But then it is possible for `onTabLinkBrowser` to get called explicitly on that tab in the window's onload handler in SessionStore, afterwards.

Ok.

> The `TabShow` event is fired when a tab's hidden state is changed; afaik this is only ever initiated by TabView when changing groups.  I am not aware of an event that gets fired when tab selection actually occurs.  Listening for `TabSelect` (which gets fired for tabbrowser tabs in `updateCurrentBrowser`) won't do because there are cases when tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).  So afaict, we'll need to create a new event (TabSelected? or TabIndexSelected?).  Should this be documented as being for lazy-browser tabs?  I am wondering that because it is a generic event which in theory could be used by something else.
> 
> So do you believe it would be better to have the handler in SessionStore rather than tabbrowser?

> The TabShow event is fired when a tab's hidden state is changed; afaik this is only ever initiated by TabView when changing groups. 

Ah, that looks right, good catch.

> Listening for TabSelect (which gets fired for tabbrowser tabs in updateCurrentBrowser) won't do because there are cases when tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).

How is that a problem? Are you saying that `TabSelect` is not triggered with `tabbox.selectedIndex` is changed programmatically?

> So do you believe it would be better to have the handler in SessionStore rather than tabbrowser?

Actually, it probably makes more sense to put it in tabbrowser. If you can figure a simple way to do this with events, go ahead, otherwise, keep it as it is (minus the `classList`), I din't think it's worth losing sleep on that.
Flags: needinfo?(dteller)
(Assignee)

Comment 183

2 years ago
https://reviewboard.mozilla.org/r/13319/#review12053

> > The TabShow event is fired when a tab's hidden state is changed; afaik this is only ever initiated by TabView when changing groups. 
> 
> Ah, that looks right, good catch.
> 
> > Listening for TabSelect (which gets fired for tabbrowser tabs in updateCurrentBrowser) won't do because there are cases when tabbox.selectedIndex is set programmatically (which occurs in the code, but I don't remember where now).
> 
> How is that a problem? Are you saying that `TabSelect` is not triggered with `tabbox.selectedIndex` is changed programmatically?
> 
> > So do you believe it would be better to have the handler in SessionStore rather than tabbrowser?
> 
> Actually, it probably makes more sense to put it in tabbrowser. If you can figure a simple way to do this with events, go ahead, otherwise, keep it as it is (minus the `classList`), I din't think it's worth losing sleep on that.

> How is that a problem? Are you saying that TabSelect is not triggered with tabbox.selectedIndex is changed programmatically?

Exactly.

> Actually, it probably makes more sense to put it in tabbrowser. If you can figure a simple way to do this with events, go ahead, otherwise, keep it as it is (minus the classList), I din't think it's worth losing sleep on that.

Using an event should be pretty simple in this case.
https://reviewboard.mozilla.org/r/13319/#review11993

> Oh, yes, I see now.  Yes, code for restore is: `tabbrowser.addTab("about:blank", {skipAnimation: true}));`

Ok, in that case, please document this.
So, what's the status? Anything I can help with?
Flags: needinfo?(allassopraise)
(Assignee)

Comment 186

2 years ago
I've been swamped with my "day job" so haven't had time to work on it.  I'll hopefully be able to spend time on it by next week.
Flags: needinfo?(allassopraise)
No problem. Just know that I have set aside some time to help you if you need, so don't hesitate to needinfo me.
(Assignee)

Comment 188

2 years ago
Okay, I'll probably be taking you up on that.
(Assignee)

Comment 189

2 years ago
https://reviewboard.mozilla.org/r/13319/#review11993

Do I need to build each time the code for the test is updated?  Or can I just drop it in place within the build? (eg, obj-ff-dbg/_tests/testing/mochitest/browser/browser/components/sessionstore/test/)
If you have just changed a test, you don't even need to drop it in place, if you call `./mach mochitest browser/components/sessionstore/test/browser_my_test.js`, it will be copied automatically.

I don't see a test in your Review Request, though. Are you sure that you have published the request? There is a (sometimes tricky to find) button "Publish".
Flags: needinfo?(allassopraise)
(Assignee)

Comment 191

2 years ago
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #190)
> I don't see a test in your Review Request, though.

I haven't made a new review request yet.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 192

2 years ago
Hello, David, I emailed you directly with some questions regarding mochitest.
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
(Assignee)

Comment 193

2 years ago
David, please check your email for subject "Re: Mochitest"
Flags: needinfo?(dteller)
We may need to wait until the browser is fully loaded. Try to add

let TOPIC = "browser-delayed-startup-finished";
yield new Promise(resolve => 
  let observer = () => {
    Services.obs.removeObserver(observer, TOPIC);
    resolve();
  };
  Services.obs.addObserver(observer, TOPIC, false);
);

before your call to `promiseBrowserState`.
Flags: needinfo?(dteller) → needinfo?(allassopraise)
Any news, Allasso?
(Assignee)

Comment 196

2 years ago
Hi David.  The only news is that I made some headway with mochitest and have been able to run the test with a multi-tab session load, but I came short of completion of writing the test.  But I am extremely busy with the principles of my life right now.  I haven't abandoned Virtual Tabs, but it has to be on the back burner for now.
Flags: needinfo?(allassopraise)

Comment 197

2 years ago
I hope this very promising patch hasn't been abandoned ;)
Allasso, if you need help, don't forget to ping me.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 199

2 years ago
Hi, David, I just dusted things off and started getting back into it a couple of days ago.  Getting re-acquainted with Mercurial, mach, mochitest (like starting over :-), and seeing how out of date my patches are now :-).  I think I'll have some time to spend with it now.  I'll sure let you know if I need help.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 200

a year ago
David, I emailed you some questions directly
Flags: needinfo?(dteller)
(Assignee)

Comment 201

a year ago
Presently the `browsers` property creates an explicit array of all tabs to their respective `linkedBrowser`.  The array is created upon first access to this property.  Doing this would immediately instantiate all browsers for all tabs.  In the current tabbrowser patch, this is modified to use a proxy instead to create a psuedo-array, which would only instantiate the browser for the tab at the index used to reference the array.

Since this change is not especially germane to this bug, I am suggesting that we assign this change to its own bug, as we did with bug 1167923.
(In reply to Allasso Travesser from comment #201)
> Since this change is not especially germane to this bug, I am suggesting
> that we assign this change to its own bug, as we did with bug 1167923.

Sounds like a good idea. Moreover, sounds like something that could already be done with a simple patch, right?

I'll respond to your e-mail later, when I have a little more time.
(Assignee)

Updated

a year ago
Depends on: 1241837
(Assignee)

Comment 203

a year ago
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #202)
> (In reply to Allasso Travesser from comment #201)
> > Since this change is not especially germane to this bug, I am suggesting
> > that we assign this change to its own bug, as we did with bug 1167923.
> 
> Sounds like a good idea. Moreover, sounds like something that could already
> be done with a simple patch, right?
Created bug 1241837
Answered.
Flags: needinfo?(dteller)
(Assignee)

Comment 205

a year ago
After communicating with David, I plan to change the strategy of this bug a little, and submit and (hopefully) land patches incrementally rather than all in one chunk.  The idea here is to first set up the framework for the lazy browser tabs, while still maintaining a working model of Firefox in the traditional manner.  Then in subsequent patches, code will be added which provides handling of code (such as in addons) that calls upon `linkedBrowser`, which, if not handled, would break things.  Next, code will be modified to exploit the lazy-browser-tab framework with the goal of obtaining startup optimization.  Finally, code will be modified in parts of the Firefox codebase which otherwise would defeat optimization by unnecessarily instantiating the browser for tabs (by accessing `linkedBrowser` properties).  At this point we will actually be realizing startup optimization.

Here is an initial rough outline of the sequence of patches, subject to modification:

1) tabbrowser.xml: Split `addTab` into two methods, `addTab` and `linkBrowserToTab`.  `addTab` will contain the minimal code to create the xul:tab only, but will not create and link the xul:browser and related elements (as it does currently).  `linkBrowserToTab` will link the xul:browser and related elements to the tab in a separate action.  This sets up the initial framework for the lazy-browser-tab.  Exploitation of this framework would look something like, calling addTab to create the tab, then some time in the future calling `linkBrowserToTab` when actually needed (such as on tab selection by the user).  In this incarnation however, we will not yet be exploiting this framework for optimization, and we will simply force a call to `linkBrowserToTab` immediately on creation of the tab.  At this point tab creation will (should) simply behave as the current `addTab` implementation.

2) tabbrowser.xml: Add a proxy to `addTab` which, if a property of `linkedBrowser` is accessed, will immediately instantiate the `linkedBrowser` for that tab.  This is in anticipation of future patch(es) where tabs may be in their "lazy" state (`linkedBrowser` not instantiated for that tab).  Without the proxy, code requiring properties of `linkedBrowser` would break since no `linkedBrowser` exists for that tab.  In this incarnation, we are still just setting up framework for potential exploitation, and we will still force linking the xul:browser to the xul:tab immediately on creation.  Again, this should not modify the functionality of Firefox.

3) SessionStore.jsm: Modify code, particularly `restoreTab`, to exploit the optimization potential of the lazy-browser-tabs.  This will also entail removing the code in tabbrowser.xml > `addTab` which has been immediately instantiating the browser on tab creation, and instead, tab selection will be the proactive action which will instantiate the xul:browser at some later time.  (NOTE: While we are now implementing code to provide optimization, in practice this optimization will not be realized due to the fact that other methods called in the Firefox codebase will access properties of `linkedBrowser` almost immediately during/after session restore which will render optimization moot.  So again, to the end user, Firefox should pretty much run as it always has.)

4) SessionStore.jsm, TabState.jsm, tabbox.xml, NetworkPrioritizer.jsm: Modify code which in the course of session restore, and in other cases, accesses properties of `linkedBrowser` and unwantedly/unnecessarily instantiates the `linkedBrowser` for tabs.  If this is successful, we should be able to do a session restore and have only the selected tab in its linkedBrowser-instantiated state, with all other tabs (regardless of the number) in their lazy state.  We should now be seeing actual optimization, and see significantly faster startup times for high volume sessions.
Would it be possible in the later part of this plan to keep around a switch to change between the 2 behaviors until we are confident there are no bugs in the wild?
Breaking tabbed browsing in a release sounds bad enough that we should have an escape for at least one release.
(Assignee)

Comment 207

a year ago
(In reply to Marco Bonardo [::mak] from comment #206)
> Would it be possible in the later part of this plan to keep around a switch
> to change between the 2 behaviors until we are confident there are no bugs
> in the wild?
> Breaking tabbed browsing in a release sounds bad enough that we should have
> an escape for at least one release.

David, can you share your thoughts about this?  I can see it would be a simple thing to have a switch which would force browser instantiation immediately, and maybe this would be all that is required.  As far as attempting to switch SessionStore code on and off, it seems it would involve quite a degree of complexity.  My hope is that by landing changes incrementally as I propose would significantly decrease the potential for a problem.
Flags: needinfo?(dteller)
(Assignee)

Comment 208

a year ago
David, I sent you a private email with some questions.
(Assignee)

Updated

a year ago
Flags: needinfo?(dteller)
(Assignee)

Updated

a year ago
Depends on: 1243707

Comment 209

a year ago
I'm seeing that all dependencies are now resolved.

Can I ask if there's anything holding this back from landing?
(Assignee)

Comment 210

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea6c64519ff
(Assignee)

Updated

a year ago
Depends on: 1276514
(Assignee)

Comment 211

a year ago
I have created bug 1276514 to deal with permanentKey implementation in lazily instantiated linkedBrowser.
See Also: → bug 1125557
Blocks: 1125557
See Also: bug 1125557

Updated

a year ago
Blocks: 1284886
Depends on: 1284947
(Assignee)

Comment 212

a year ago
How does this block 1284886?
(Assignee)

Updated

a year ago
Depends on: 1285789
(Assignee)

Comment 213

a year ago
I have created bug 1285789 to start implementing event to signal when browser has been linked to the tab.
Keywords: footprint
Priority: -- → P2
Hardware: x86 → All
Version: unspecified → Trunk

Comment 214

a year ago
Great work. I'm really looking forward to this.
(Assignee)

Updated

a year ago
Depends on: 1287330
(Assignee)

Comment 215

a year ago
I have created bug 1287330.  This is the next step which adds a proxy for linkedBrowser to ensure things don't get broken by a consumer of linkedBrowser on a tab still in the lazy-browser state.

Note that in that 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.
(Assignee)

Updated

11 months ago
Depends on: 1287456
(Assignee)

Updated

11 months ago
Depends on: 1287479

Comment 216

11 months ago
Sorry for spamming this thread with my petty comments and questions, but is there a way to apply this patch to a current version of FF and get a reasonably stable browser out if it (one which most importantly does not corrupt sessionstore.js)? I can't wait to give it a try!
(Assignee)

Comment 217

11 months ago
(In reply to timm.friedholz from comment #216)
> Sorry for spamming this thread with my petty comments and questions, but is
> there a way to apply this patch to a current version of FF and get a
> reasonably stable browser out if it (one which most importantly does not
> corrupt sessionstore.js)? I can't wait to give it a try!

It would be far from stable at this point.

Comment 218

11 months ago
(In reply to Allasso Travesser from comment #217)
> It would be far from stable at this point.

But Some people's maybe ready to try it in current condition (including me)
(Assignee)

Updated

11 months ago
Depends on: 1289370
(Assignee)

Comment 219

11 months ago
Soon we will be modifying SessionStore to begin implementing the optimization provided by lazy-browser tabs.  Before doing that, call sites which access tab.linkedBrowser must be prepared so tabs in lazy-browser state are handled properly.

I have created bug 1289370 to create a series of patches to do that.

Comment 220

11 months ago
Allasso, I admire your perseverance and the slow but steadily increasing success you obtain in that issue.

I would love to see the lightweight tabs rather sooner than later in action. I'm willing to test any work-in-progress version of your changes. Maybe you have a mercurial repository of a mozilla fork available, where your current version of the patches is publicly available (e.g. as a separate branch) such that I can regularily compile and test (and use) your patches? I would be willing to give up all Firefox add-ons I use for the benefit of using your patches. (Or I would fix the add-ons to make them compatible with lazy-browser tabs.)

I would also like to mention the issue of tab-unloading: once lightweight tabs are possible, it would be desirable to be able to convert a heavyweight tab back into a lightweight tab again. This is the intention of add-ons such as https://addons.mozilla.org/de/firefox/addon/auto-unload-tab/ . Such an extension would then just need to call a function like "makeLightweight" in order to convert a heavyweight tab into a lightweight tab (in whichever way that is implemented).

Once conversion from lightweight to heavyweight and back works, the "live bookmarks" usage pattern can be used sustainably: The user uses an elaborate tab organization add-on, for example https://addons.mozilla.org/de/firefox/addon/tree-style-tab/ to organize tabs hierarchically (where tabs pertaining related topics are organized closely to each other), optionally grouping tabs into windows, one window per overarching topic. Only currently or recently used tabs get promoted to heavyweight, once a tab "expires" (user-configurable in the auto-unload-tab add-on), the tab gets demoted to lightweight automatically. This would allow users to use this pattern without requiring 18 gigabytes of RAM and a permanently running fan caused by Firefox' garbage collector which is constantly traversing this amount of RAM hoping in vain to find RAM which could be freed.
(Assignee)

Comment 221

11 months ago
Xuan, the patches that have been created up to now still do not result in any optimization.  What we have been working on is laying the foundation so that when optimization finally does become a reality, the tabs built under the new API will be handled properly.  If this were not done first, everything would be quite a mess.  The code that will actually result in optimization will be the introduced in the final stage.

Regarding tab-unloading, please see bug 1284886.  This bug already hints at creating an "unload" tab API which will utilize lazy-browser optimization.
(Assignee)

Comment 222

11 months ago
Created attachment 8778688 [details] [diff] [review]
Patch EXPERIMENTAL ONLY 1

Experimental patch exploring SessionStore handling of lazy browser tabs, and satellite consumers.
Attachment #8632252 - Attachment is obsolete: true
Attachment #8634113 - Attachment is obsolete: true
(Assignee)

Comment 223

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ff9392f9b1
(Assignee)

Comment 224

11 months ago
Created attachment 8778715 [details] [diff] [review]
Patch EXPERIMENTAL ONLY 2
(Assignee)

Comment 225

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ef9da0a469

Comment 226

10 months ago
i've filed bug 1299505 for a narrower idea.

Updated

9 months ago
Duplicate of this bug: 1304644
(Assignee)

Comment 228

8 months ago
Allasso Travesser is now Kevin Jones.
Blocks: 1330635

Updated

4 months ago
Blocks: 1341474
(Assignee)

Updated

4 months ago
Depends on: 1342723

Updated

4 months ago
Blocks: 1139608
(Assignee)

Updated

4 months ago
Depends on: 1345090
(Assignee)

Updated

4 months ago
Depends on: 1345098

Updated

4 months ago
Duplicate of this bug: 1256472
(Assignee)

Updated

3 months ago
Depends on: 1352157
(Assignee)

Updated

3 months ago
Depends on: 1352183

Updated

3 months ago
Blocks: 1348289
Alias: lazytabs
We would like to land bug 1322485 in time for 57, and that depends on bug 1284886, which possibly depends on this bug. To get an idea of the likelihood that this sequence of bugs could land in time for 57, can anyone involved here comment on the likelihood of this bug landing well before the 57 merge date? That will give us an idea if we can make bug 1284886 depend on this, or if we need to consider other implementations.

Updated

3 months ago
No longer blocks: 1284886
(Assignee)

Comment 231

3 months ago
Much work has been done in recent months on this bug, final patches for bug 1345090 are being reviewed and hopefully this bug will land shortly[1].  bug 1345090 is a support bug for this bug (the motherbug), and is the final major component of the lazy-browser tabs API.  Once bug 1345090 lands, lazy-browser tabs will become a working part of Firefox.

[1] :dao and :mikedeboer would be better qualified to give light on this.  Dao or Mike, and either of you comment on this?
(Assignee)

Comment 232

3 months ago
^^^^

Sorry, disregard - I didn't catch Dao's update on this.
Regardless, it wouldn't surprise me if this were to become a Quantum Flow blocker. (Kevin, if you don't know what this is, I'd be happy to explain this to you privately. Not because it's secret, but simply it's a longer story and bugs are not an ideal venue for proze.)
This means that we'll make an effort to get this in as soon as possible. Whatever is needed, we'll try to provide it. At the moment this is mostly reviews and architectural advice. But that may become more in the near future.
I'm hoping to enable this feature in Fx 56, because we can use the time catch any possible regressions from a big change like this.
Whiteboard: [qf:p1]
Depends on: 1354596
Comment hidden (off-topic)
Comment hidden (off-topic)
(Assignee)

Comment 236

2 months ago
(In reply to Brendan from comment #234)
> The irony of this is that, as I see it, the main benefit of this change is
> that it will make it easier to use large numbers of tabs.  But it looks like
> the fix is not going to be complete until after XUL extensions are dropped,
> so the main extensions that are also needed for using large numbers of tabs
> (such as the very useful Tab Groups Helper written by the main person
> working on this bug) will no longer be available, and it will still not be
> possible to comfortably use large numbers of tabs in Firefox.  Is that right
> or am I missing something?

It appears there is work in the direction of tab groups in bug 1333837 (see comment - #2, also bug 1322057).
Comment hidden (off-topic)
(Assignee)

Updated

2 months ago
Attachment #8778688 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8778715 - Attachment is obsolete: true

Updated

2 months ago
Keywords: meta
No longer depends on: 1354596
Depends on: 1354614
(Assignee)

Comment 238

2 months ago
Note to Nightly testers:

Bug 1345098 just landed with a patch that will help monitor if lazy browser tabs are being prematurely instantiated (and thus no longer lazy).  If this happens the message "[bug 1345098] Lazy browser prematurely inserted via '${name}' property access:" will be printed out in the JS console, along with a stack trace showing the code that triggered it.

Comment 239

2 months ago
If the instantiation is triggered by an addon is it still worthwhile to report which property is being accessed or should we just take it to the addon authors?
(Assignee)

Comment 240

2 months ago
(In reply to The 8472 from comment #239)
> If the instantiation is triggered by an addon is it still worthwhile to
> report which property is being accessed or should we just take it to the
> addon authors?

It would still be worthwhile to report it.  Depending on the property which is instantiating the browser, we may be able to provide a trap which might make things easier if it is something other addons may typically want to access.

I would just report it as a comment in bug 1345098 rather than opening a new bug on it.
(In reply to Kevin Jones from comment #240)
> I would just report it as a comment in bug 1345098 rather than opening a new
> bug on it.

Bugs are cheap. Please do file new bugs so that we can track individual issues properly. Otherwise bug 1345098 would likely become very messy soon.

Updated

2 months ago
Depends on: 1360148

Updated

2 months ago
Depends on: 1360173
(Assignee)

Comment 242

2 months ago
Dao, I was thinking that it would be good if there was a way to check if any WebExtensions APIs are triggering early browser instantiation.  Short of manually checking every single test, can you think of a mechanism we can set up which would tell us if a WE API is triggering instantiation?
(Assignee)

Comment 243

2 months ago
^^^^

Sorry if this is a dup, but it didn't appear n'info got added
Flags: needinfo?(dao+bmo)
Depends on: 1360239
Depends on: 1360314

Updated

2 months ago
Depends on: 1360323
(In reply to Kevin Jones from comment #242)
> Dao, I was thinking that it would be good if there was a way to check if any
> WebExtensions APIs are triggering early browser instantiation.  Short of
> manually checking every single test, can you think of a mechanism we can set
> up which would tell us if a WE API is triggering instantiation?

I think the feedback we get from bug 1345098 is sufficient.
Flags: needinfo?(dao+bmo)

Comment 245

2 months ago
So I mentioned in bug 1360148#c5 that all content processes get started on FF shutdown.
Is this behavior normal?

I was thinking it might be another addon issue, but I'm not sure. Before this bug landed all tabs were created on startup, somewhere.

Comment 246

2 months ago
I think this has caused a regression where the switch-to-tab feature (Ctrl+L, % <tab title>) of the awesome bar does not work any longer for unloaded tabs.
(Assignee)

Comment 247

2 months ago
(In reply to avada from comment #245)
> So I mentioned in bug 1360148#c5 that all content processes get started on
> FF shutdown.
> Is this behavior normal?

No.  This does not seem to occur on vanilla Firefox.
(Assignee)

Updated

2 months ago
Depends on: 1360932
(Assignee)

Comment 248

2 months ago
(In reply to The 8472 from comment #246)
> I think this has caused a regression where the switch-to-tab feature
> (Ctrl+L, % <tab title>) of the awesome bar does not work any longer for
> unloaded tabs.

Thanks for the report.  I filed bug 1360932.

Comment 249

2 months ago
Created attachment 8863256 [details]
Error console, when lazy browsers are inserted during shutdown.

(In reply to Kevin Jones from comment #247)
> (In reply to avada from comment #245)
> > So I mentioned in bug 1360148#c5 that all content processes get started on
> > FF shutdown.
> > Is this behavior normal?
> 
> No.  This does not seem to occur on vanilla Firefox.

Well, I managed to make a screenshot. See attachment.
(Assignee)

Updated

2 months ago
Depends on: 1360940
(Assignee)

Comment 250

2 months ago
(In reply to avada from comment #249)
> Created attachment 8863256 [details]
> Error console, when lazy browsers are inserted during shutdown.
> 
> (In reply to Kevin Jones from comment #247)
> > (In reply to avada from comment #245)
> > > So I mentioned in bug 1360148#c5 that all content processes get started on
> > > FF shutdown.
> > > Is this behavior normal?
> > 
> > No.  This does not seem to occur on vanilla Firefox.
> 
> Well, I managed to make a screenshot. See attachment.

Thank you Avada.  A change was made to browser.js/CanCloseWindow() some time since Nightly build 20170428030259 causing all lazy browsers to be instantiated on shutdown.  I filed bug 1360940.

Comment 251

2 months ago
Wasn't the tab groups addon mentioned before in regards of prematurely inserting lazy browsers?

Was it determined if it's an issue on FF's side or the addon's (like with BackTrack Tab History)?

Comment 252

2 months ago
It looks like the "Close other tabs" feature which calls "gBrowser.removeAllTabsBut(TabContextMenu.contextTab);"
also results in all unloaded tabs to be inserted into content processes. As such it might take a long time to close those tabs.
As does close tabs to the right/left. But at least the extra content processes are killed after the tabs are closed.

I see an error message like this in these cases:

[bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2126:45
onTabClosing@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/backtrack@byalexv.co.uk.xpi!/bootstrap.js:1164:9
onSSTabClosing@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/backtrack@byalexv.co.uk.xpi!/bootstrap.js:1094:5
ssi_onTabClose@resource:///modules/sessionstore/SessionStore.jsm:1919:5
ssi_handleEvent@resource:///modules/sessionstore/SessionStore.jsm:1001:11
_beginRemoveTab@chrome://tabmixplus/content/changecode.js line 220 > eval:80:13
removeTab@chrome://browser/content/tabbrowser.xml:2643:18
change_gBrowser/gBrowser.removeTab@chrome://tabmixplus/content/minit/tablib.js:255:14
addNewFunctionsTo_gBrowser/gBrowser.closeRightTabs@chrome://tabmixplus/content/minit/tablib.js:1255:13
addNewFunctionsTo_gBrowser/gBrowser._closeRightTabs@chrome://tabmixplus/content/minit/tablib.js:1238:9
oncommand@chrome://browser/content/browser.xul:1:1
  tabbrowser.xml:2126
(Assignee)

Updated

2 months ago
Depends on: 1362092
(Assignee)

Comment 253

2 months ago
(In reply to avada from comment #252)
> It looks like the "Close other tabs" feature which calls
> "gBrowser.removeAllTabsBut(TabContextMenu.contextTab);"
> also results in all unloaded tabs to be inserted into content processes. As
> such it might take a long time to close those tabs.
> As does close tabs to the right/left. But at least the extra content
> processes are killed after the tabs are closed.
> 
> I see an error message like this in these cases:
> 
> [bug 1345098] Lazy browser prematurely inserted via 'messageManager'
> property access:
> getter@chrome://browser/content/tabbrowser.xml:2126:45
> onTabClosing@resource://gre/modules/addons/XPIProvider.jsm ->
> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.
> teszt-teljes/extensions/backtrack@byalexv.co.uk.xpi!/bootstrap.js:1164:9
> onSSTabClosing@resource://gre/modules/addons/XPIProvider.jsm ->
> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.
> teszt-teljes/extensions/backtrack@byalexv.co.uk.xpi!/bootstrap.js:1094:5
> ssi_onTabClose@resource:///modules/sessionstore/SessionStore.jsm:1919:5
> ssi_handleEvent@resource:///modules/sessionstore/SessionStore.jsm:1001:11
> _beginRemoveTab@chrome://tabmixplus/content/changecode.js line 220 >
> eval:80:13
> removeTab@chrome://browser/content/tabbrowser.xml:2643:18
> change_gBrowser/gBrowser.removeTab@chrome://tabmixplus/content/minit/tablib.
> js:255:14
> addNewFunctionsTo_gBrowser/gBrowser.closeRightTabs@chrome://tabmixplus/
> content/minit/tablib.js:1255:13
> addNewFunctionsTo_gBrowser/gBrowser._closeRightTabs@chrome://tabmixplus/
> content/minit/tablib.js:1238:9
> oncommand@chrome://browser/content/browser.xul:1:1
>   tabbrowser.xml:2126

This caused by Backtrack addon.  I've filed bug 1362092.

Comment 254

2 months ago
(In reply to Kevin Jones from comment #253)
> This caused by Backtrack addon.  I've filed bug 1362092.

Cool. And it's already fixed now. :)


On a different note. I ran into a brutal issue. I tried overwriting a large session with hundreds of tabs with a session that only had two tabs, via Session Manager. Content processes started to open like crazy, well past the limit set by dom.ipc.processCount. Looked like hundreds, probably as much as the number of tabs in the session to be overwritten.
This shouldn't be possible even with addons, right?

Checked the error console filtering to 1345098 which pointed to findbar tweak causing the insertions, but doesn't explain the number of content processes:

"[bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2126:45
messageBrowser@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/fbt@quicksaver.xpi!/bootstrap.js -> resource://findbartweak/modules/utils/Modules.jsm -> resource://findbartweak/modules/utils/Messenger.jsm:88:1
unloadFromBrowser@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/fbt@quicksaver.xpi!/bootstrap.js -> resource://findbartweak/modules/utils/Modules.jsm -> resource://findbartweak/modules/utils/Messenger.jsm:143:3
handleEvent@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/fbt@quicksaver.xpi!/bootstrap.js -> resource://findbartweak/modules/utils/Modules.jsm -> resource://findbartweak/modules/findInTabsMini.jsm:30:5
handleEvent@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/fbt@quicksaver.xpi!/bootstrap.js -> resource://findbartweak/modules/utils/Modules.jsm -> resource://findbartweak/modules/utils/Listeners.jsm:59:6
_beginRemoveTab@chrome://tabmixplus/content/changecode.js line 220 > eval:80:13
removeTab@chrome://browser/content/tabbrowser.xml:2643:18
change_gBrowser/gBrowser.removeTab@chrome://tabmixplus/content/minit/tablib.js:255:14
ssi_restoreWindow@resource:///modules/sessionstore/SessionStore.jsm:3410:7
Modules.LOADMODULE/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Zsolt/AppData/Roaming/Mozilla/Firefox/Profiles/q74vpos5.teszt-teljes/extensions/tabgroups@quicksaver.xpi!/bootstrap.js -> resource://tabgroups/modules/utils/Modules.jsm -> resource://tabgroups/modules/Storage.jsm:155:3
ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:3532:5
ssi_setBrowserState@resource:///modules/sessionstore/SessionStore.jsm:2237:5
ss_setBrowserState@resource:///modules/sessionstore/SessionStore.jsm:258:5
restoreSession@chrome://sessionmanager/content/modules/session_data_processing.jsm:141:5
restoreSession@chrome://sessionmanager/content/modules/session_data_processing.jsm:36:10
load/<@chrome://sessionmanager/content/modules/session_file_io.jsm:521:16
run@chrome://sessionmanager/content/modules/utils.jsm:675:9
"  tabbrowser.xml:2126
(Assignee)

Updated

2 months ago
Depends on: 1363078

Updated

2 months ago
Depends on: 1363240

Updated

a month ago
Depends on: 1368193

Comment 255

a month ago
hmm, something changed.

with just about:memory and about:performance open in tabs a content process is always running, it previously dropped to just the parent.
You need to log in before you can comment on or make changes to this bug.