Tabbar would not scroll properly after session is restored. The current tab is not displayed in the tabbar.

VERIFIED FIXED in Firefox 54

Status

()

Core
Layout
--
major
VERIFIED FIXED
3 months ago
20 days ago

People

(Reporter: Alice0775 White, Assigned: mconley)

Tracking

({regression, ux-consistency})

54 Branch
mozilla55
All
Windows
regression, ux-consistency
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox53 unaffected, firefox54+ verified, firefox55 verified, firefox-esr52 unaffected)

Details

(Whiteboard: [STR: see Comment#16])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 months ago
Created attachment 8841445 [details]
sessionstore.js

[Tracking Requested - why for this release]: The current tab is missing after session restore. Tabbar would not scroll properly after session is restored

Reproducible : always

Steps To Reproduce:
1. Open many tabs (50-60tabs)
2. Open several new tab after [web page2]
   [web page1][web page2][New Tab1][New Tab2][New Tab3][web page3]....[web pageN]
3. Select [New Tab1]
4. Quit and re-start Nightly and then Restore Previous Session

Actual Results:
No selected tab is displayed in the Tabbar.

Expected Results:
The current tab should be displayed in the Tabbar.
(Reporter)

Comment 1

3 months ago
Also reproduced on Aurora53.0a2.

Steps To Reproduce:
1. Open many tabs (50-60tabs)
2. Open several new tab after [web page2]
   [web page1][web page2][New Tab1][New Tab2][New Tab3][web page3]....[web pageN]
3. Select [New Tab1]
4. Quit and re-start Nightly and then Restore Previous Session
5. (optionally, Repeat step4 if needed)

Actual Results:
After Stap4. New tabs will be removed due to Bug 1306294(maybe by design right now).
However, Current selected tab is missing in the tabbar. Tabbar scroll position is wrong.

Expected Results:
The Current selected tab should be visible in the tabbar.


The bunch of patch (Bug 1323987 and Bug 1306294) should be backed out.
Blocks: 1306294
status-firefox53: unaffected → affected
tracking-firefox53: --- → ?
(Reporter)

Updated

3 months ago
Keywords: ux-consistency
(Reporter)

Comment 2

3 months ago
I can reproduce the issue even if only 20 tabs are opened.

Updated

3 months ago
Blocks: 1343056

Updated

3 months ago
status-firefox51: --- → unaffected
Has Regression Range: --- → yes
Has STR: --- → yes
OS: Windows 10 → Windows
Hardware: x86 → All
Version: 54 Branch → 53 Branch
Flags: needinfo?(mlongaray)
(Assignee)

Comment 3

3 months ago
Hey mikedeboer,

Just giving you a heads up on this. How should we proceed?
Flags: needinfo?(mdeboer)
This is expected to happen on version 53 (current Aurora) since when we first landed bug 1306294 we were removing about:newtab and about:blank tabs from session state (see https://hg.mozilla.org/mozilla-central/rev/cbe1b40baf57#l3.25).

Bug 1323987 was filed to solve this issue and now we're not removing such tabs from session state (see https://hg.mozilla.org/mozilla-central/rev/997080796cd3).

I used attached sessionstore.js file on version 54 (current Nightly) and could not reproduce the issue, as expected.

The issue can be reproduced on version 53 because (using attached sessionstore.js file) the current/selected tab is an about:newtab tab, thus, such tab will not be displayed in the tab bar (since we were removing such kind of tab from bug 1306294).

One thing we could do is to uplift commit https://hg.mozilla.org/mozilla-central/rev/997080796cd3 to Aurora as soon as possible before merge.

What do you think, Mike? Feel free to confirm what I just said. Please let me know what I can do to help.
Flags: needinfo?(mlongaray)
(In reply to Matheus Longaray (:mlongaray) from comment #4)
> What do you think, Mike? Feel free to confirm what I just said. Please let
> me know what I can do to help.

I also think we'll need to uplift these patches to 53 asap. Do you need help with that?
Flags: needinfo?(mdeboer) → needinfo?(mlongaray)
(Reporter)

Comment 6

3 months ago
(In reply to Matheus Longaray (:mlongaray) from comment #4)

> 
> I used attached sessionstore.js file on version 54 (current Nightly) and
> could not reproduce the issue, as expected.
> 

I can reproduce the problem on Noghtly54.0a1 With STR in comment#0
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Matheus Longaray (:mlongaray) from comment #4)
> > What do you think, Mike? Feel free to confirm what I just said. Please let
> > me know what I can do to help.
> 
> I also think we'll need to uplift these patches to 53 asap. Do you need help
> with that?

I've never requested an uplift. I see that https://wiki.mozilla.org/Release_Management/Uplift_rules contains info in order to do that. Should I proceed?
Flags: needinfo?(mlongaray)
At this point it's also quite important to figure out _why_ Alice is able to reproduce it just fine and you can't using the same STR...
(Assignee)

Comment 9

3 months ago
I concur. We need to understand what's going on here before we consider an uplift.
(Reporter)

Comment 10

3 months ago
screen cast: https://youtu.be/3XQHh3xz3Fw
Current tab is not shown in tabbar after session restore.

Workaround, resize browser, then the current tab appears in tabbar.
So, per Alice's screen cast, we can see that current tab is properly restored and selected after restoring previous session.

I see that our tabbar UI did not 're-flowed' itself to show the current tab though the selected tab's content was correct.

Are we sure that the regression range is correct?
PS: current tab appears in tabbar on Linux (local development environment).

Comment 13

3 months ago
(In reply to Matheus Longaray (:mlongaray) from comment #12)
> PS: current tab appears in tabbar on Linux (local development environment).

can confirm works on linux but not in windows10 RS2
Managed to reproduce the issue on Linux as well. After restoring session, navigated tabbar until the end, closed the browser, restored previous session -> tabbar UI did not re-flowed itself to show the current tab (however, tab's content was correct and was not removed from session state - no data loss).

Alice's workaround also caused expected effect on Linux.
Any thoughts, mikedeboer?
Flags: needinfo?(mdeboer)
(Reporter)

Comment 16

3 months ago
regression-windowstr
Okay,
The STR is not good. The New tab thing hides the real culprit.


Steps To Reproduce(without New tab):
1. Open many tabs (20tabs is enough)
   [about:home][https://developer.mozilla.org/en-US/Add-ons/Code_snippets][about:home]....[about:home][about:home]
2. Select 2nd tab
3. Restart Browser, and then Restore Previous Session

Actual Results:
Tabbar scrolls at wrong position, so the current tab is not shown in the tabbar.

Expected results:
Tabbar should scroll so the current tab will be shown in the tabbar.



Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=69404501d4d8fc26cc4317ccaec8cf997e7b599a&tochange=910ceca7e8637c4c3ea963fd054e6c449eeff6a7

Regressed by:
910ceca7e863	Olli Pettay — Bug 1315570, vsync handling in parent process' main thread should use high priority runnables, r=ehsan



(:mconley, :mikedeboer, :mlongaraySorry , sorry for the confusion)
Blocks: 1315570
No longer blocks: 1343056, 1306294, 1323987
status-firefox53: affected → unaffected
tracking-firefox53: ? → ---
Component: Tabbed Browser → Layout
Flags: needinfo?(mdeboer) → needinfo?(bugs)
Product: Firefox → Core
Version: 53 Branch → 54 Branch
(Reporter)

Updated

3 months ago
Whiteboard: [STR: see Comment#16]
^5 Alice, thanks for the update! I'll be keeping an eye on this bug.
(Assignee)

Comment 18

3 months ago
Thanks for clearing this up, Alice!
Tracking 54+ for this regression, so that the tab functionality works as expected.
tracking-firefox54: ? → +

Updated

2 months ago
Depends on: 1346644

Comment 20

2 months ago
Created attachment 8846458 [details] [diff] [review]
wip

This helps, and as expected, the patch shows that we have some racy code somewhere and prioritizing vsync (requestAnimationFrame callbacks) triggers different execution for the racy code. 

Better would be of course to fix the racy code. Whether it has something to do with reflow and XBL constructors or some setTimeout somewhere or requestAnimationFrame... dunno yet.

Is this kind of patch acceptable, or does anyone familiar with this code have ideas how else to fix this?
Flags: needinfo?(bugs) → needinfo?(mlongaray)

Comment 21

2 months ago
Or maybe Mike knows
Flags: needinfo?(mdeboer)
I'm afraid I'm not well familiar with this piece of code. Maybe ttaubert can help you out as he also helped me with SessionStore related-code before.
Flags: needinfo?(mlongaray) → needinfo?(ttaubert)
Mike's already CC'ed. He owns SessionStore these days :)
Flags: needinfo?(ttaubert)
Comment on attachment 8846458 [details] [diff] [review]
wip

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

Apology for my late reply here - I'm not yet 100% familiar with the code here, so I needed a bit more time to think about it.

Thanks for the patience!

::: browser/components/sessionstore/SessionStore.jsm
@@ +3450,5 @@
>      tabsDataArray.length = numTabsInWindow;
>  
>      // If provided, set the selected tab.
>      if (aSelectTab > 0 && aSelectTab <= aTabs.length) {
> +      let selectedTab = aTabs[aSelectTab - 1];

Hm, what I'd be OK with is to have a `setTimeout` at the end of this function and you keep a reference to the tab to select here:
```js
let tabToSelect = null;
if (aSelectTab > 0 && aSelectTab <= aTabs.length) {
  tabToSelect = aTabs[aSelectedTab - 1];
}
// ... rest of method ...
if (tabToSelect) {
  tabbrowser.ownerDocument.defaultView.setTimeout(() =>
    tabbrowser.selectedTab = selectedTab;
});
```

This is because I'm expecting more deferred operation to be added here very soon.
Attachment #8846458 - Flags: feedback+
Flags: needinfo?(mdeboer)

Comment 25

2 months ago
Created attachment 8847121 [details] [diff] [review]
patch

Let's see how many tests will be broken
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1ee0cb98540e74af3d767c0b363e05042b42404
Assignee: nobody → bugs
Attachment #8846458 - Attachment is obsolete: true
Attachment #8847121 - Flags: review?(mdeboer)

Comment 26

2 months ago
Created attachment 8847122 [details] [diff] [review]
patch

Right patch this time
Attachment #8847121 - Attachment is obsolete: true
Attachment #8847121 - Flags: review?(mdeboer)
Attachment #8847122 - Flags: review?(mdeboer)
(Reporter)

Updated

2 months ago
status-firefox55: --- → affected
Comment on attachment 8847122 [details] [diff] [review]
patch

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

So this is breaking quite a number of tests, it seems :( Not entirely surprising, but myeah, quite annoying.

OK, does this work when you just move the `tabbrowser.selectedTab = tabToSelect;` outside of the loop without the timeout?
Attachment #8847122 - Flags: review?(mdeboer)

Comment 28

2 months ago
You mean just not using setTimeout? That doesn't work.
bug 1315570 made rAF callbacks and layout to run possibly earlier, all asynchronous.
(It is now disabled in Bug 1346644 because of this bug)
:'(

OK, moving on. I'm taking a quick look if we can do something smart about the tests. Meanwhile, do you think we can use a timer which finishes in the same stack as a Promise that resolves? Or simply use

```js
new Promise(resolve => {
  tabbrowser.selected = tabToSelect;
  resolve();
});
```

I know that the methods to propagate the sessionstore with tests data, restore windows & tabs use promises to wait for all the things. Assertion come after that and since timers are not reliable instruments, I'd conjure that this'd improve our situation.
(In reply to Mike de Boer [:mikedeboer] from comment #29)
> ```js
> new Promise(resolve => {
>   tabbrowser.selected = tabToSelect;
>   resolve();
> });
> ```

This doesn't work. After crawling through code and flows a bit here, I stumbled upon the 'switcher' in tabbrowser.xml, which reacts to a whole bunch of events to keep the UI up-to-date asynchronously.
I think at this point it'd be OK to ask mconley what we can do here: Mike, what do you think the tab switcher can do wrt Olli's findings in comment 20?
Flags: needinfo?(mconley)

Comment 31

2 months ago
(Promises are synchronous from Gecko point of view. They run within the current task.)
(Assignee)

Comment 32

2 months ago
Hm, I think I know what's going on here.

SessionStore.jsm's restoreWindow method calls addTab when it's restoring state into a window that doesn't have enough tabs (which is true in the STR here). So a bunch of new tabs are added.

We're smart enough not to animate those tabs on creation - however, we also queue a bunch of setTimeout(f, 0)'s to fire soon after the <xul:tab> is added into the DOM:

http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/browser/base/content/tabbrowser.xml#2273

So those setTimeout's are all queued, but haven't fired by the time we reach here:

http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/browser/components/sessionstore/SessionStore.jsm#3436-3446

Which sets the selected tab to the one we actually want selected and visible, and then starts restoring the history state into the tabs.

Yes, we have an async tab switcher for e10s to account for layer trees not yet being ready from the content process - however, from the perspective of <xul:tabs>, the tab switch _is_ synchronous (there's an arguably unfortunate difference between "logically selected" and "visually selected" in this model).

So we've selected the tab, and at this point, because it's one of the first two tabs (using the STR from comment 16), the _handleTabSelect code (which fires once the tab is selected):

http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/browser/base/content/tabbrowser.xml#5912-5918

Decides that the tab is (rightly) still visible, and so we don't scroll anywhere.

Then those runnables we queued start firing. Those are calling into _handleNewTab. For the tabs _after_ the selected tab, we enter this condition:

http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/browser/base/content/tabbrowser.xml#6362

which enters here:

http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/browser/base/content/tabbrowser.xml#6227-6277

and that determines that the tab that we just added in the background is something that the user probably cares about and scrolls it into view. It does that for each tab that gets added. This ends up scrolling us to the end of the tab strip.

I don't know why smaug's patch caused this, to be honest. Those setTimeout's were always destined to run after the initial tab selection.

Anyhow, I have a patch that fixes this - and it might even give us a little performance improvement along the way, as we get to skip a lot of unnecessary work in _notifyBackgroundTab for restored tabs.
Assignee: bugs → mconley
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)

Comment 34

2 months ago
mozreview-review
Comment on attachment 8849851 [details]
Bug 1342849 - Don't do any notifications for newly added background tabs when restoring session.

https://reviewboard.mozilla.org/r/122586/#review125034

Well, this will certainly help perf! (More than you think at the moment, probably, especially since you're eliminating a sizeable chunk of heavy-duty code; I see multiple getBoundingClientRect() calls and a `_calcTabMargins` call, for example.)

In `_notifyBackgroundTab` the attribute name 'notifybgtab' is used. That's used on the scroll button, but I'd vote for that one to be renamed to something more self-explanatory and non-confusing.

Thanks!

::: browser/base/content/tabbrowser.xml:2263
(Diff revision 1)
>                ContextualIdentityService.setTabStyle(t);
>              }
>  
>              t.setAttribute("onerror", "this.removeAttribute('image');");
> +
> +            if (aSkipBackgroundNotify) {

Please add this attribute to `ATTRIBUTES_TO_SKIP` in TabAttributes.jsm

::: browser/base/content/tabbrowser.xml:6376
(Diff revision 1)
>            this.adjustTabstrip();
>  
>            if (tab.getAttribute("selected") == "true") {
>              this._fillTrailingGap();
>              this._handleTabSelect();
> -          } else {
> +          } else if (!tab.getAttribute("skipbackgroundnotify")) {

Please use `hasAttribute` here
Attachment #8849851 - Flags: review?(mdeboer) → review+
The late(r) review was due to me being sick this week... still am a little. Just sayin' ;-)
(Assignee)

Comment 36

2 months ago
mozreview-review-reply
Comment on attachment 8849851 [details]
Bug 1342849 - Don't do any notifications for newly added background tabs when restoring session.

https://reviewboard.mozilla.org/r/122586/#review125034

Just so I'm clear, you want me to rename the notifybgtab attribute to something else (here, and in the CSS that selects on it)? What works - "backgroundtabnotification"? "highlight"?
(Assignee)

Comment 37

2 months ago
Thanks for the review! ni? for comment 36.
Flags: needinfo?(mdeboer)
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #36)
> Just so I'm clear, you want me to rename the notifybgtab attribute to
> something else (here, and in the CSS that selects on it)? What works -
> "backgroundtabnotification"? "highlight"?

Yeah, 'highlight' works nicely, I think! (This is also the animation I'd like to disable during session restore, btw, just like we do with the others)
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request)

Comment 40

2 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/838e5ace9f13
Don't do any notifications for newly added background tabs when restoring session. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 42

2 months ago
(In reply to Wes Kocher (:KWierso) from comment #41)
> https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13

This seems to have brought a regression .
STR is still wip

but basically if there are 20 or so tabs(unloaded)
quickly ctrl+tab between them results a different tab being selected while ctrl+tab already starts to load another tab while still being on another.

Happens mostly switching from last tab to first.

Sorry if not clear
(Assignee)

Comment 43

2 months ago
(In reply to Sunil Kumar from comment #42)
> (In reply to Wes Kocher (:KWierso) from comment #41)
> > https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13
> 
> This seems to have brought a regression .
> STR is still wip
> 
> Sorry if not clear

Hey Sunil, thanks for the report! I think I understand what you're describing.

Are you certain that the patch for this bug caused the issue, and it did not exist in earlier builds? I ask, because what you're describing is a bug that I think has existed for a while, and wasn't just introduced.

If you take a March 20th Nightly, for example:

https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-mozilla-central/

(Here's the Windows zip: https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-mozilla-central/firefox-55.0a1.en-US.win32.zip )

do you still see the behaviour in that build?

Comment 44

2 months ago
(In reply to Mike Conley (:mconley) (Offsite until March 27) from comment #43)
> (In reply to Sunil Kumar from comment #42)
> > (In reply to Wes Kocher (:KWierso) from comment #41)
> > > https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13
> > 
> > This seems to have brought a regression .
> > STR is still wip
> > 
> > Sorry if not clear
> 
> Hey Sunil, thanks for the report! I think I understand what you're
> describing.
> 
> Are you certain that the patch for this bug caused the issue, and it did not
> exist in earlier builds? I ask, because what you're describing is a bug that
> I think has existed for a while, and wasn't just introduced.
> 
> If you take a March 20th Nightly, for example:
> 
> https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> mozilla-central/
> 
> (Here's the Windows zip:
> https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> mozilla-central/firefox-55.0a1.en-US.win32.zip )
> 
> do you still see the behaviour in that build?

Thanks for getting it was not sure how to make the issue understandable

Got this after today's build, maybe just hit it today not sure.

okay testing it out and if will comment here.
status-firefox-esr52: --- → unaffected

Comment 45

2 months ago
(In reply to Mike Conley (:mconley) (QF workweek until March 31) from comment #43)
> (In reply to Sunil Kumar from comment #42)
> > (In reply to Wes Kocher (:KWierso) from comment #41)
> > > https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13
> > 
> > This seems to have brought a regression .
> > STR is still wip
> > 
> > Sorry if not clear
> 
> Hey Sunil, thanks for the report! I think I understand what you're
> describing.
> 
> Are you certain that the patch for this bug caused the issue, and it did not
> exist in earlier builds? I ask, because what you're describing is a bug that
> I think has existed for a while, and wasn't just introduced.
> 
> If you take a March 20th Nightly, for example:
> 
> https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> mozilla-central/
> 
> (Here's the Windows zip:
> https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> mozilla-central/firefox-55.0a1.en-US.win32.zip )
> 
> do you still see the behaviour in that build?

Yes you were right!!
Just produced it on the build you provided.
Any fix for it yet?

Also found two bug which reproduced after landing this patch(or maybe some other patch landed with this one)

1. New profile, default settings, browser.sessionstore.restore_on_demand false,started to record performance profile while the session was being restored by loading 3 tabs at once in the background and I was slowly scrolling the page in the active tab. The result: http://i.imgur.com/wbBJln6.png
As you can see, fps constantly drops to 2-3 frames per second, the scrolling doesn't feel smooth at all

& browser.sessionstore.restore_on_demand true

cntrl + tabs quickly to load 10+ tabs and after most tabs load same thing as above.

2. multiple bookmarks in multiple folders say 10 in each folder
e10s =on and not in non e10s

Click folder to open all bookmarks
quickly followed by folder 2 then 3 then 4 etc

tab update to show bookmarks opening but sometimes tabs disappear 
eg Folder 1 and 2 all bookmarks open but folder 3 bookmarks appear to load and disappear(or 4 or 5) , folder 4 5 open fine.
clicking folder 3 after other tabs open load it fine.

some-throttling  tabs in e10s is happening when opening many bookmarks(and way to slow compared to e10s off  )

Any ideas?

sorry if not clear.
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(mconley)
(In reply to Sunil Kumar from comment #45)
> Also found two bug which reproduced after landing this patch(or maybe some
> other patch landed with this one)

Please file new bugs for these bugs, and please note (if possible) if they reproduce prior to the patch in this bug landing.  Thanks!
(Assignee)

Comment 48

2 months ago
Comment on attachment 8849851 [details]
Bug 1342849 - Don't do any notifications for newly added background tabs when restoring session.

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1315570

[User impact if declined]:

Selected tab might not scroll into view after a window is restored (for example, when the session is restored during start-up).

[Is this code covered by automated tests?]:

No.

[Has the fix been verified in Nightly?]:

Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

Couldn't hurt. STR are in comment 16.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

It's a small scoped change that makes us skip doing a thing we don't need to do, which was causing the tab strip to scroll when it shouldn't have.

[String changes made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8849851 - Flags: approval-mozilla-aurora?

Comment 49

2 months ago
(In reply to Mike Conley (:mconley) (Very high latency until April 3rd) from comment #43)
> (In reply to Sunil Kumar from comment #42)
> > (In reply to Wes Kocher (:KWierso) from comment #41)
> > > https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13
> > 
> > This seems to have brought a regression .
> > STR is still wip
> > 
> > Sorry if not clear
> 
> Hey Sunil, thanks for the report! I think I understand what you're
> describing.
> 
> Are you certain that the patch for this bug caused the issue, and it did not
> exist in earlier builds? I ask, because what you're describing is a bug that
> I think has existed for a while, and wasn't just introduced.
> 
> If you take a March 20th Nightly, for example:
> 
> https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> mozilla-central/
> 
> (Here's the Windows zip:
> https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> mozilla-central/firefox-55.0a1.en-US.win32.zip )
> 
> do you still see the behaviour in that build?

(In reply to Sunil Kumar from comment #45)
> (In reply to Mike Conley (:mconley) (QF workweek until March 31) from
> comment #43)
> > (In reply to Sunil Kumar from comment #42)
> > > (In reply to Wes Kocher (:KWierso) from comment #41)
> > > > https://hg.mozilla.org/mozilla-central/rev/838e5ace9f13
> > > 
> > > This seems to have brought a regression .
> > > STR is still wip
> > > 
> > > Sorry if not clear
> > 
> > Hey Sunil, thanks for the report! I think I understand what you're
> > describing.
> > 
> > Are you certain that the patch for this bug caused the issue, and it did not
> > exist in earlier builds? I ask, because what you're describing is a bug that
> > I think has existed for a while, and wasn't just introduced.
> > 
> > If you take a March 20th Nightly, for example:
> > 
> > https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> > mozilla-central/
> > 
> > (Here's the Windows zip:
> > https://ftp.mozilla.org/pub/firefox/nightly/2017/03/2017-03-20-03-02-09-
> > mozilla-central/firefox-55.0a1.en-US.win32.zip )
> > 
> > do you still see the behaviour in that build?
> 
> Yes you were right!!
> Just produced it on the build you provided.
> Any fix for it yet?
> 
> Also found two bug which reproduced after landing this patch(or maybe some
> other patch landed with this one)
> 
> 1. New profile, default settings, browser.sessionstore.restore_on_demand
> false,started to record performance profile while the session was being
> restored by loading 3 tabs at once in the background and I was slowly
> scrolling the page in the active tab. The result:
> http://i.imgur.com/wbBJln6.png
> As you can see, fps constantly drops to 2-3 frames per second, the scrolling
> doesn't feel smooth at all
> 
> & browser.sessionstore.restore_on_demand true
> 
> cntrl + tabs quickly to load 10+ tabs and after most tabs load same thing as
> above.
> 
> 2. multiple bookmarks in multiple folders say 10 in each folder
> e10s =on and not in non e10s
> 
> Click folder to open all bookmarks
> quickly followed by folder 2 then 3 then 4 etc
> 
> tab update to show bookmarks opening but sometimes tabs disappear 
> eg Folder 1 and 2 all bookmarks open but folder 3 bookmarks appear to load
> and disappear(or 4 or 5) , folder 4 5 open fine.
> clicking folder 3 after other tabs open load it fine.
> 
> some-throttling  tabs in e10s is happening when opening many bookmarks(and
> way to slow compared to e10s off  )
> 
> Any ideas?
> 
> sorry if not clear.

Like you said this is a know bug, has it been filed anywhere where STR can be posted?
Hi Alice,
could you help verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
(Reporter)

Comment 51

2 months ago
(In reply to Gerry Chang [:gchang] from comment #50)
> Hi Alice,
> could you help verify if this issue was fixed as expected on a latest
> Nightly build? Thanks!

no longer reproduce the problem on Nightly55.0a1[1].

[1]
https://hg.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170403030207
Flags: needinfo?(alice0775)
(Reporter)

Updated

2 months ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Comment on attachment 8849851 [details]
Bug 1342849 - Don't do any notifications for newly added background tabs when restoring session.

Fix a regression related to scrolling issue in selected tab after session restore and was verified. Aurora54+.
Attachment #8849851 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 53

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5587c1bbdf94
status-firefox54: affected → fixed
Let's make sure this works as intended on Fx54 as well. Details available in Comment 16.
Flags: qe-verify+
I managed to reproduce the issue on Firefox 54.0a1 (2017-02-26), under Windows 10x64, using the STR from Comment 16.
The issue is no longer reproducible on Firefox 54.0b5.
Tests were performed under Windows 10x64 and under Windows 8.1x32.
status-firefox54: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.