900803 - SessionStore opens tabs one by one, triggering synchronous reflow each time

Status: RESOLVED DUPLICATE of bug 1242912

(Firefox :: Session Restore, defect)

Description

[Steven MacLeod [:smacleod]]

|restoreWindow()| calls |addTab()| repeatedly, creating and inserting tabs one by one, triggering reflows. We should create all the tabs, and then add them together all at once.

This work has been split out of Bug 816607, which deals with closing tabs one by one.

Comment 1

[Dão Gottwald [:dao]]

copying over a relevant comment from bug 816607:

>+            // inject the new DOM nodes
>+            this.tabContainer.appendChild(tabsFragment);
>+            this.mPanelContainer.appendChild(browsersFragment);
>+            this.tabContainer.updateVisibility();
>+
>+            for (let i = 0; i < tabs.length; i++) {
>+              let t = tabs[i], b = browsers[i];
>+
>+              // wire up a progress listener for the new browser object.
>+              let tabListener = this.mTabProgressListener(t, b, true);
>+              const filter = Components.classes["@mozilla.org/appshell/component/browser-status-filter;1"]
>+                                       .createInstance(Components.interfaces.nsIWebProgress);
>+              filter.addProgressListener(tabListener, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
>+              b.webProgress.addProgressListener(filter, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
>+              this.mTabListeners[startPosition + i] = tabListener;
>+              this.mTabFilters[startPosition + i] = filter;
>+
>+              b.droppedLinkHandler = handleDroppedLink;
>+
>+              // Dispatch a new tab notification.  We do this once we're
>+              // entirely done, so that things are in a consistent state
>+              // even if the event listener opens or closes tabs.
>+              let evt = document.createEvent("Events");
>+              evt.initEvent("TabOpen", true, false);
>+              t.dispatchEvent(evt);

You're dispatching TabOpen for the individual tabs at a point where you already added various other tabs that didn't get the TabOpen event yet. That's basically breaking the API.

Comment 2

[Steven MacLeod [:smacleod]]

(In reply to Dão Gottwald [:dao] from comment #1)
> You're dispatching TabOpen for the individual tabs at a point where you
> already added various other tabs that didn't get the TabOpen event yet.
> That's basically breaking the API.

I'm not sure I see it as breaking the API. Is it documented somewhere that only the single tab has been added when that event fires?

Are you aware of any code that is making this assumption, and would need to be fixed? I don't see an obvious way around this, while still fixing the problem of reflows for each tab.

Comment 3

[tabmix.onemen]

Is it possible to dispatch new event at the start and end of addBlankTabs

Comment 4

[Dão Gottwald [:dao]]

(In reply to Steven MacLeod [:smacleod] from comment #2)
> (In reply to Dão Gottwald [:dao] from comment #1)
> > You're dispatching TabOpen for the individual tabs at a point where you
> > already added various other tabs that didn't get the TabOpen event yet.
> > That's basically breaking the API.
> 
> I'm not sure I see it as breaking the API. Is it documented somewhere that
> only the single tab has been added when that event fires?

https://developer.mozilla.org/en-US/docs/Web/Reference/Events/TabOpen

"The TabOpen event is executed when a tab is opened."

> Are you aware of any code that is making this assumption, and would need to
> be fixed?

Any code that uses the event to keep track of tabs and also accesses e.g. tabbrowser.[tabs|visibleTabs|browsers] or tabContainer.childNodes is potentially affected, as the code's internal representation would be out of sync with the additional tabs suddenly exposed by tabbrowser. I haven't looked if we have code where this would be a problem. As the assumption made by such code seems pretty reasonable to me, I don't think "fixing" that code would be the way to go. We could only do it for our own code anyway, not for add-ons.

> I don't see an obvious way around this, while still fixing the
> problem of reflows for each tab.

Do we know what code causes layout flushes when adding tabs? Would it be feasible to introduce a "batch tab adding mode" in which we would skip such operations?

(In reply to onemen.one from comment #3)
> Is it possible to dispatch new event at the start and end of addBlankTabs

I don't see how this would magically make TabOpen work like it's currently expected to work.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Could we just fire N TabOpen events at the end of opening N tabs?

Comment 6

[Steven MacLeod [:smacleod]]

(In reply to Dão Gottwald [:dao] from comment #4)
> https://developer.mozilla.org/en-US/docs/Web/Reference/Events/TabOpen
> 
> "The TabOpen event is executed when a tab is opened."

I believe this new code is true to that description. For each tab that is opened, a TabOpen event is fired. I don't see anything about guarantees of only a single new tab existing.

> > Are you aware of any code that is making this assumption, and would need to
> > be fixed?
> 
> Any code that uses the event to keep track of tabs and also accesses e.g.
> tabbrowser.[tabs|visibleTabs|browsers] or tabContainer.childNodes is
> potentially affected, as the code's internal representation would be out of
> sync with the additional tabs suddenly exposed by tabbrowser. I haven't
> looked if we have code where this would be a problem. As the assumption made
> by such code seems pretty reasonable to me, I don't think "fixing" that code
> would be the way to go. We could only do it for our own code anyway, not for
> add-ons.

FWIW, this doesn't seem to cause any issues in tests (After fixing a number of bugs the currently posted patch has). The breadth of my knowledge in this area is lacking though so I am also unsure if we'd see any problems.

> 
> > I don't see an obvious way around this, while still fixing the
> > problem of reflows for each tab.
> 
> Do we know what code causes layout flushes when adding tabs? Would it be
> feasible to introduce a "batch tab adding mode" in which we would skip such
> operations?

I believe it is just the nodes for the tabs being inserted causing the reflows. I don't really have the expertise to answer about a "batch tab adding mode". Could anyone else weigh in on this?

> (In reply to onemen.one from comment #3)
> > Is it possible to dispatch new event at the start and end of addBlankTabs
> 
> I don't see how this would magically make TabOpen work like it's currently
> expected to work.

^+1

(In reply to Jared Wein [:jaws] from comment #5)
> Could we just fire N TabOpen events at the end of opening N tabs?

That's what the current patch is doing, so the change is on the first TabOpen event to fire there are N new tabs, instead of 1.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Steven MacLeod [:smacleod] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > https://developer.mozilla.org/en-US/docs/Web/Reference/Events/TabOpen
> > 
> > "The TabOpen event is executed when a tab is opened."
> 
> I believe this new code is true to that description. For each tab that is
> opened, a TabOpen event is fired. I don't see anything about guarantees of
> only a single new tab existing.

The description says "when a tab is opened", not "some time after the tab was added with random other tabbrowser operations possibly happening in the meantime."

> > Any code that uses the event to keep track of tabs and also accesses e.g.
> > tabbrowser.[tabs|visibleTabs|browsers] or tabContainer.childNodes is
> > potentially affected, as the code's internal representation would be out of
> > sync with the additional tabs suddenly exposed by tabbrowser. I haven't
> > looked if we have code where this would be a problem. As the assumption made
> > by such code seems pretty reasonable to me, I don't think "fixing" that code
> > would be the way to go. We could only do it for our own code anyway, not for
> > add-ons.
> 
> FWIW, this doesn't seem to cause any issues in tests (After fixing a number
> of bugs the currently posted patch has). The breadth of my knowledge in this
> area is lacking though so I am also unsure if we'd see any problems.

Yeah, I don't think we can rely on tests to expose such issues.

> > Do we know what code causes layout flushes when adding tabs? Would it be
> > feasible to introduce a "batch tab adding mode" in which we would skip such
> > operations?
> 
> I believe it is just the nodes for the tabs being inserted causing the
> reflows.

Just to make sure we're on the same page, by "reflow" you mean layout being (1) marked dirty and (2) flushed, and inserting the tab DOM node does both of these things?

Comment 8

[tabmix.onemen]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to onemen.one from comment #3)
> > Is it possible to dispatch new event at the start and end of addBlankTabs
> 
> I don't see how this would magically make TabOpen work like it's currently
> expected to work.

I didn't meant it to replace TabOpen, just new events for "batch tab adding started" and "batch tab adding finished"
it can use for example to skip doing some actions until the batch tab adding finished.

regarding TabOpen can you add listener to DOMNodeInserted then when tabsFragment append to tabContainer catch DOMNodeInserted from each tab and dispatch TabOpen

Comment 9

[Dão Gottwald [:dao]]

(In reply to onemen.one from comment #8)
> regarding TabOpen can you add listener to DOMNodeInserted then when
> tabsFragment append to tabContainer catch DOMNodeInserted from each tab and
> dispatch TabOpen

That's an interesting idea. I'm not sure if that would actually avoid the problem, but it sounds like it might. Unfortunately, DOM mutation event listeners slow down all DOM mutations in a window and are therefore pretty much disallowed where performance matters in our code base.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #9)
> That's an interesting idea. I'm not sure if that would actually avoid the
> problem, but it sounds like it might. Unfortunately, DOM mutation event
> listeners slow down all DOM mutations in a window and are therefore pretty
> much disallowed where performance matters in our code base.

MutationObservers don't have this problem, right? (There's some overhead, to be sure, but it doesn't have permanent effects on the document.)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Just to make sure we're on the same page, by "reflow" you mean layout being
> (1) marked dirty and (2) flushed, and inserting the tab DOM node does both
> of these things?

We should use reflow observers (as in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_tabopen_reflows.js) to compare what happens with Steven's patch vs. without.

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > That's an interesting idea. I'm not sure if that would actually avoid the
> > problem, but it sounds like it might. Unfortunately, DOM mutation event
> > listeners slow down all DOM mutations in a window and are therefore pretty
> > much disallowed where performance matters in our code base.
> 
> MutationObservers don't have this problem, right?

MutationObserver callbacks are called with queued mutations, so they won't be of use here, as I understand it.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #4)
> Any code that uses the event to keep track of tabs and also accesses e.g.
> tabbrowser.[tabs|visibleTabs|browsers] or tabContainer.childNodes is
> potentially affected, as the code's internal representation would be out of
> sync with the additional tabs suddenly exposed by tabbrowser. I haven't
> looked if we have code where this would be a problem. As the assumption made
> by such code seems pretty reasonable to me, I don't think "fixing" that code
> would be the way to go. We could only do it for our own code anyway, not for
> add-ons.

I'm having a hard time coming up with example cases where this would be a problem - having fewer tabs than expected when the event fires would obviously be problematic, but having additional tabs seems less likely to break things.

Feels like we should try it, and see what breaks.

While we're at it, adding an additional event to indicate the start/end of a batched addition of tabs, as onemen suggested, is probably a good idea (so that anyone who is affected has a way to work around it - we can determine later whether that is sufficient).

Comment 14

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #13)
> I'm having a hard time coming up with example cases where this would be a
> problem - having fewer tabs than expected when the event fires would
> obviously be problematic, but having additional tabs seems less likely to
> break things.

You just needs some code adding tabs to an array in response to TabOpen, like <http://hg.mozilla.org/mozilla-central/annotate/1ed5a88cd4d0/browser/base/content/browser-tabPreviews.js#l496>. Now it could e.g. check the new tab's previous/nextSibling's position in the array to insert the new tab next to it, but if previous/nextSibling are not in the list it gets into an unexpected state. Or it could listen to other tab events like TabAttrModified, TabSelect, TabMove or TabPinned. Other code could, in response to a TabOpen event, move, pin, select or otherwise touch various /other/ tabs, and then our code would get events for those tabs before they were added to the array, which again puts it in an unexpected state that currently cannot happen.

I don't think we should try this and see what breaks. Even if we discover nothing, you can tell a priori that it weakens the tabbrowser API and can lead to subtle and obscure bugs possibly involving multiple API consumers.

If we can't figure out how to speed this up sanely, I feel we shouldn't do it at all. The performance improvement should only become noticeable with (I'm being optimistic) dozens if not (more likely true) hundreds of tabs, which we know isn't the case for the vast majority of users.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #14)
> You just needs some code adding tabs to an array in response to TabOpen,
> like
> <http://hg.mozilla.org/mozilla-central/annotate/1ed5a88cd4d0/browser/base/
> content/browser-tabPreviews.js#l496>. Now it could e.g. check the new tab's
> previous/nextSibling's position in the array to insert the new tab next to
> it, but if previous/nextSibling are not in the list it gets into an
> unexpected state. Or it could listen to other tab events like
> TabAttrModified, TabSelect, TabMove or TabPinned. Other code could, in
> response to a TabOpen event, move, pin, select or otherwise touch various
> /other/ tabs, and then our code would get events for those tabs before they
> were added to the array, which again puts it in an unexpected state that
> currently cannot happen.

The code you link to isn't broken by this, right? And the other scenarios you list seem possible but improbable. I don't doubt that this would break _someone_, but it's hard to judge whether that negative impact outweighs the benefits of this change (particularly if we have a mitigation in place where affected code can switch to using additional events) without knowing what is broken exactly.

> I don't think we should try this and see what breaks. Even if we discover
> nothing, you can tell a priori that it weakens the tabbrowser API and can
> lead to subtle and obscure bugs possibly involving multiple API consumers.

If in practice no one is relying on this behavior, this would not be a significant "weakening" of the API IMO. It does introduce a divergence in behavior (between the multiple-add case and the single-add case), which increases complexity somewhat. But the behavior within those cases is consistent, and a way to easily distinguish them (e.g. another event) would probably mitigate most potential confusion.

> If we can't figure out how to speed this up sanely, I feel we shouldn't do
> it at all. The performance improvement should only become noticeable with
> (I'm being optimistic) dozens if not (more likely true) hundreds of tabs,
> which we know isn't the case for the vast majority of users.

There is a benefit for any >1 tab session. You're right that the benefit scales directly with the size of the session, but I don't think that's a reason to give up. It may be the the compatibility hit is too high for this to be worth it, but I don't think we should pre-judge that.

Comment 16

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> The code you link to isn't broken by this, right?

I haven't checked, because that was besides the point. I was merely pointing out the pattern of keeping track of tabs via TabOpen/TabClose and listening to various other tab events. For instance, you could easily imagine the TabSelect listener being broken when calling this.detachTab for a tab this code didn't know about yet, if detachTab never had to deal with tabs being absent from the internal array before.

> And the other scenarios you list seem possible but improbable.

Why improbable? Running code in response to TabOpen that affects other tabs, thereby triggering more tab events, seems like a perfectly reasonable pattern for an add-on utilizing the APIs we provide.

> > I don't think we should try this and see what breaks. Even if we discover
> > nothing, you can tell a priori that it weakens the tabbrowser API and can
> > lead to subtle and obscure bugs possibly involving multiple API consumers.
> 
> If in practice no one is relying on this behavior, this would not be a
> significant "weakening" of the API IMO. It does introduce a divergence in
> behavior (between the multiple-add case and the single-add case), which
> increases complexity somewhat. But the behavior within those cases is
> consistent, and a way to easily distinguish them (e.g. another event) would
> probably mitigate most potential confusion.

No, another event as proposed in comment 3 doesn't help at all with the cases I described.

> There is a benefit for any >1 tab session. You're right that the benefit
> scales directly with the size of the session, but I don't think that's a
> reason to give up.

So, how much does it help?

Comment 17

[Steven MacLeod [:smacleod]]

(In reply to Dão Gottwald [:dao] from comment #16)
> So, how much does it help?

I took a couple of measurements. Time values for "Before" and "After" patch are in ms, and measurements were taken on a MacBook Pro Retina, 2.7 GHz i7, 16GB, OSX 10.8.4

Value of SESSION_RESTORE_RESTORE_WINDOW_MS in ms
# Of Tabs   Before  After
1   8   8
1   7   8
1   8   7
1   9   8
1   8   8
6   48  41
6   47  41
6   51  42
6   46  44
6   47  42
11  76  69
11  76  68
11  78  67
11  79  69
11  79  66
21  140 114
21  141 116
21  138 115
21  139 118
21  141 117
41  277 213
41  277 214
41  277 217
41  276 215
41  276 216

Comment 18

[Steven MacLeod [:smacleod]]

Out of curiosity, I took the same measurements on the UX branch, both before and after applying the patch. It appears there is an even greater effect.

# Of Tabs   Before  After
1   8   5
1   6   8
1   5   5
1   8   6
1   5   6
6   50  43
6   49  44
6   51  45
6   50  44
6   48  44
11  87  73
11  88  74
11  87  71
11  84  69
11  85  70
21  162 122
21  160 125
21  161 125
21  156 126
21  161 125
41  319 225
41  319 221
41  315 225
41  324 225
41  322 224

Comment 19

[Steven MacLeod [:smacleod]]

Attachment: WIP Patch - API for adding multiple blank tabs at once, and switching restore window to use it

Discussion has kind of died here and I've stopped actively working on this. I've updated the WIP patch to the latest m-c and I'm uploading it as-is. Not really sure what we've decided we'd like to do going forward.

Any more thoughts?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

The perf wins seem obvious to me, we just need to figure out what to do with our events. Is that a good summary of the state of this bug?

Comment 21

[Steven MacLeod [:smacleod]]

(In reply to Jared Wein [:jaws] from comment #20)
> The perf wins seem obvious to me, we just need to figure out what to do with
> our events. Is that a good summary of the state of this bug?

Yes. There doesn't appear to be any way around the problem Dao brought up though, so I think it's either we make the change and see if anything breaks, or discard it.

Comment 22

[Dão Gottwald [:dao]]

There's no question that there's a win, but it still seems marginal to me for the vast majority of our users.

Comment 23

[Steven MacLeod [:smacleod]]

Attachment: WIP Patch - API for adding multiple blank tabs at once, and switching restore window to use it

Rebased patch

Comment 24

[Steven MacLeod [:smacleod]]

Gavin and I talked and agreed with Dao it's not the right thing to just land this and see what breaks. I've rebased the patch to leave up, and I'm going to stop maintaining it. Maybe this can be revisited in the future.