Closed Bug 1559690 Opened 3 months ago Closed 3 months ago

Tabbar with pinned tabs working incorrectly on linux after bug 1488953

Categories

(Core :: Layout, defect, P1)

69 Branch
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected

People

(Reporter: svanderger, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Kubuntu 18.04.1 LTS (updated)

  1. Create new profile
  2. Uncheck "Title bar" on "Customize..." page
  3. Check "Restore previous session" on "about:preferences" page
  4. Open few tabs and pin some count (22 in my case)
  5. Restart browser (few times maybe)

Actual results:

A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: chrome://global/content/bindings/scrollbox.xml:761

Mozregression pointed on this
Last good revision: c5379d7e3e1953bf91b0513b90be9892945b3314
First bad revision: 6b9cfebe807648f7c68f5a618f54cb0633298045
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c5379d7e3e1953bf91b0513b90be9892945b3314&tochange=6b9cfebe807648f7c68f5a618f54cb0633298045

Component: Untriaged → Layout
Product: Firefox → Core
Regressed by: 1488953

Thanks for reporting!

I'd want to reproduce this, but

(In reply to John from comment #0)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Kubuntu 18.04.1 LTS (updated)

  1. Create new profile
  2. Uncheck "Title bar" on "Customize..." page

I don't quite understand this step. Is it something specific for Kubuntu? I am on GNOME so there is no such options?

Flags: needinfo?(svanderger)

Never mind, I could reproduce this.
Probably _updateScrollButtonsDisabledState in scrollbox.xml is called recursively somehow.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(svanderger)
Priority: -- → P1

This is more or less a problem in tabbrowser.xml. The recursive calls seems to be triggered by getBoundingClientRect() for scrollButtonDown element and I thought replacing it with the cached value so that this problem disappears, BUT all pinned tabs are invisible there. :/ I think rendering pinned tabs somehow relies on the getBoundingClientRect() call there.

Attached patch A possible fixSplinter Review

Probably this is a reasonable fix (I hope!) to avoid the recursive calls. At least this patch fixes the issue locally.

A problem I've noticed is that we set scrollButtonWidth property which calls getBoundingClientRect() for the scroll button, and we repeatedly call the getBoundingClientRect() in a loop for the number of the pinned tabs.

So I think we can set the scrollButtonWidth property value to "immediate" value (I don't know immediate is the right word or not), we can avoid the number of the calls of getBoundingClientRect().

Dao, does this patch sounds reasonable?

Attachment #9072935 - Flags: feedback?(dao+bmo)
Comment on attachment 9072935 [details] [diff] [review]
A possible fix

The expression after `scrollButtonWidth:` doesn't run lazily but when the object is declared. So I don't understand how this patch would make any difference.
Attachment #9072935 - Flags: feedback?(dao+bmo) → feedback-

Thanks, then I have no idea how to fix this recursive hell for now. Moving into browser component to make this issue noticeable for browser people.

Component: Layout → Tabbed Browser
Product: Core → Firefox

Can you elaborate on where exactly there's a "recursive hell" and how this is a front-end bug rather than a layout one?

Flags: needinfo?(hikezoe)

Well, the recursive hell happens somewhere in between scrollbox.xml and tabbox.xml, especially places they handle scroll, scrollend, overflow and underflow events (there might be others). Before bug 1488953, overflow and underflow events were being fired just right before we start painting , bug now the events are fired on the same machinery which is used for other normal events. So from what I can tell, the current browser and the toolkit's scrollbox codes rely on the assumption that overflow and underflow events are separately fired from other events. That seems clearly wrong to me.

Flags: needinfo?(hikezoe)

"somewhere in between scrollbox.xml and tabbox.xml" is rather unspecific, and I haven't been able to reproduce this bug yet.

What exactly is the new model for overflow and underflow? Do they fire after painting, or something else?

Keywords: steps-wanted

I've seen this happen now, got these errors:

JavaScript error: chrome://browser/content/tabbrowser.xml, line 554: too much recursion
JavaScript error: chrome://browser/content/tabbrowser.xml, line 403: too much recursion
JavaScript error: chrome://global/content/bindings/scrollbox.xml, line 284: too much recursion

(In reply to Dão Gottwald [::dao] from comment #9)

What exactly is the new model for overflow and underflow? Do they fire after painting, or something else?

Emilio, can you maybe shed some light on this?

Flags: needinfo?(emilio)

They fire after layout, but I guess they should really use the same mechanism as Scroll / ScrollEnd events use, which ticks off the refresh driver, otherwise it's probably too footgunny. The scroll area events do the same but...

Also, I think this code is wrong now: https://searchfox.org/mozilla-central/rev/c0ca77697c6868482f30af873ec8069f2c080a34/layout/generic/nsGfxScrollFrame.cpp#4617

Hiro, I think we should use the refresh driver's scroll event queue instead, does that seem reasonable to you? If so I can write the patch.
Alternatively would you be ok backing out the bug that regresses this for now?

Flags: needinfo?(emilio) → needinfo?(hikezoe)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

They fire after layout, but I guess they should really use the same mechanism as Scroll / ScrollEnd events use, which ticks off the refresh driver, otherwise it's probably too footgunny. The scroll area events do the same but...

Also, I think this code is wrong now: https://searchfox.org/mozilla-central/rev/c0ca77697c6868482f30af873ec8069f2c080a34/layout/generic/nsGfxScrollFrame.cpp#4617

Hiro, I think we should use the refresh driver's scroll event queue instead, does that seem reasonable to you? If so I can write the patch.
Alternatively would you be ok backing out the bug that regresses this for now?

Yes, using the refresh driver's event queue makes totally sense to me. I actually started it to try to solve the oranges happened in bug 1488953, but soon after I realized a flaw in the test itself in question in bug 1488953. That's said I am not sure using the event queue solve this recursive hell, but let's give it a shot! If it doesn't work, I am fine with backing out the patch. :)

Flags: needinfo?(hikezoe)

I suspect it will.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

This seems to work for me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5654b7b25a2f80a70cc045608373a4c340158e35

This is running through try, though if Hiro or John could confirm it works there too it'd be great.

Flags: needinfo?(emilio)

This prevents them from being called from layout and is more similar to what
WillPaintObservers did.

Depends on D35603

Component: Tabbed Browser → Layout
Keywords: steps-wanted
Product: Firefox → Core

The issue with using the same mechanism as scroll events is that overflow events
that result from a layout flush from the refresh driver tick don't happen until
the next refresh driver tick. That's bad and causes a few tests that don't
wait enough to fail. I guess the next alternative is what PostDOMEvent uses,
which is just dispatch to the event loop.

This is actually green and also fixes the bug for the same reason.

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/ae4b3c9f8ee5
Move scroll event declarations to nsGfxScrollFrame.cpp r=hiro
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/fdb96c16d976
Make scroll port events use the same mechanism as scroll events. r=hiro
https://hg.mozilla.org/integration/autoland/rev/9f45982e7800
Actually make scrollport events use the event loop, since using the same scroll event mechanism breaks too much stuff. r=hiro

Backed out 2 changesets for causing failures in browser_tabCloseSpacer.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/918d7c84da371bda26460fb4b50f161d32107880

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=253547668&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=9f45982e7800871078d5b707bcfcc6c12000bbc0&searchStr=%28bc3

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253547668&repo=autoland&lineNumber=18769

16:43:44 INFO - TEST-PASS | browser/base/content/test/tabs/browser_tabCloseSpacer.js | Close button hasn't moved - {"left":858,"top":8,"width":20,"height":20} deepEqual {"left":858,"top":8,"width":20,"height":20} -
16:43:44 INFO - Buffered messages finished
16:43:44 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_tabCloseSpacer.js | Close button hasn't moved - {"left":918,"top":8,"width":20,"height":20} deepEqual {"left":858,"top":8,"width":20,"height":20} - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/tabs/browser_tabCloseSpacer.js :: <TOP_LEVEL> :: line 31
16:43:44 INFO - Stack trace:
16:43:44 INFO - chrome://mochitests/content/browser/browser/base/content/test/tabs/browser_tabCloseSpacer.js:null:31
16:43:44 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1115
16:43:44 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1143
16:43:44 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1004
16:43:44 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

Flags: needinfo?(emilio)

Hmm the change makes browser_tabCloseSpacer.js perma fail?
Emilio, I am totally fine with backing out bug 1488953. WDYT?

Yes, let's do that.

Flags: needinfo?(emilio)

Should be fixed by backout.

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Duplicate of this bug: 1560814
Attachment #9073776 - Attachment is obsolete: true
Attachment #9073381 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.