Closed Bug 1513007 Opened Last year Closed Last year

Scrolling new background tabs into view can stop working completely if we ever try to scroll a tab into view when the document doesn't need a layout/style flush

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + verified
firefox66 + verified

People

(Reporter: dao, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

No description provided.
Flags: needinfo?(gijskruitbosch+bugs)
I can't reproduce, can you provide more detailed steps?

bug 1483354 is listed as having made 65, so I assume you meant to mark that as affected, too?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dao+bmo)
1. pin a tab
2. open enough tabs for the tab bar to overflow
3. select the last tab
4. open a background tab

This reproduces pretty consistently over here except for one time, so this appears to be somewhat intermittent.
Flags: needinfo?(dao+bmo)
It seems there is some case (presumably somehow to do with layout state, where perhaps the callback can run immediately if the layout state is fully clean anyway?) where we don't clear this._backgroundTabScrollPromise, which means we never re-enter the if () statement at https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/browser/base/content/tabbrowser.xml#938 .

Off-hand, I'm not 100% sure how to fix this. The theory of the current code is:

1) we only ever have 1 pending promiseDocumentFlushed() callback/promise.
2) as soon as we get layout info for that one, we clear the promise guard so that any subsequent tab opens will re-run the scroll logic.

Moving the clearing to be later (e.g. in the promise resolved callback) AIUI runs the risk of a new tab scroll instruction coming in after we've computed bounds for the last-to-be-selected tab, and that scroll being "dropped".

I guess I can track the last-opened tab while the promise is pending, and on promise resolution, if there is another tab that isn't the same, just re-invoke the scroll code, but that feels a bit yucky...
(In reply to :Gijs (he/him) from comment #3)
> It seems there is some case (presumably somehow to do with layout state,
> where perhaps the callback can run immediately if the layout state is fully
> clean anyway?)

https://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindowInner.cpp#6661-6664

  UniquePtr<PromiseDocumentFlushedResolver> flushResolver(
      new PromiseDocumentFlushedResolver(resultPromise, aCallback));

  if (!shell->NeedStyleFlush() && !shell->NeedLayoutFlush()) {
    flushResolver->Call();
    return resultPromise.forget();
  }


So yes, that's possible. I'll come up with something...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
I was just about to file a bug about the same issue, except I can reproduce it without having to pin tab(s).
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
Oops, re-setting.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs (he/him) from comment #3)
> I guess I can track the last-opened tab while the promise is pending, and on
> promise resolution, if there is another tab that isn't the same, just
> re-invoke the scroll code, but that feels a bit yucky...

... but I don't think we've got anything better. Dão, I've put up a patch, and it seems to wfm, but I'd appreciate if you doublechecked it as well, given that I couldn't reproduce the issue as reliably as you (for whatever reason).
Summary: Scrolling new background tabs into view has stopped working when there are pinned tabs → Scrolling new background tabs into view can stop working completely if we ever try to scroll a tab into view when the document doesn't need a layout/style flush
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c5d4b4f1e496
avoid permanently breaking scrolling to background tabs when promiseDocumentFlushed returns synchronously, r=dao
https://hg.mozilla.org/mozilla-central/rev/c5d4b4f1e496
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Duplicate of this bug: 1514071
Comment on attachment 9030279 [details]
Bug 1513007 - avoid permanently breaking scrolling to background tabs when promiseDocumentFlushed returns synchronously, r?dao

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1483354

User impact if declined: Links opened in new tabs get lost as soon as the tabstrip overflows

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment #2. It may need more than 1 attempt to reproduce the issue.

List of other uplifts needed: n/a

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's early in beta, and this directly modifies the same code as bug 1483354. We have time to back the whole pile out if we discover further issues

String changes made/needed: none
Attachment #9030279 - Flags: approval-mozilla-beta?
Comment on attachment 9030279 [details]
Bug 1513007 - avoid permanently breaking scrolling to background tabs when promiseDocumentFlushed returns synchronously, r?dao

[Triage Comment]
Fixes an annoying regression with tab strip scrolling when new tabs are loaded. Approved for 65.0b5.
Attachment #9030279 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Can you please clarify the scrolling part? Because the only difference I see between the affected build and the fixed one is that on the affected build when a new background tab is opened it's not automatically scrolled into view, you have to navigate manually on the tabs bar. Is this how the issue was reproducing? Or am I doing something wrong?
Flags: needinfo?(dao+bmo)
(In reply to Oana Botisan from comment #15)
> Can you please clarify the scrolling part? Because the only difference I see
> between the affected build and the fixed one is that on the affected build
> when a new background tab is opened it's not automatically scrolled into
> view, you have to navigate manually on the tabs bar. Is this how the issue
> was reproducing?

Yes.
Flags: needinfo?(dao+bmo)
Thanks for the info.
I managed to reproduce the issue using an older version of Nightly (2018-12-10) on Windows 10 x64.
I verified the fix using latest Nightly 66.0a1 and beta 65.0b5 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The bug is not reproducing anymore.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 1514405
You need to log in before you can comment on or make changes to this bug.