1559690 - Tabbar with pinned tabs working incorrectly on linux after bug 1488953

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[John]

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: A possible fix

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?

Comment 5

[Dão Gottwald [:dao]]

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.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 9

[Dão Gottwald [:dao]]

"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?

Comment 10

[Dão Gottwald [:dao]]

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

Comment 11

[Dão Gottwald [:dao]]

(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?

Comment 12

[Emilio Cobos Álvarez [: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?

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(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. :)

Comment 14

[Emilio Cobos Álvarez [:emilio]]

I suspect it will.

Comment 15

[Emilio Cobos Álvarez [: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.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1559690 - Move scroll event declarations to nsGfxScrollFrame.cpp r=hiro

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1559690 - Make scroll port events use the same mechanism as scroll events. r=hiro

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

Depends on D35603

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1559690 - Actually make scrollport events use the event loop, since using the same scroll event mechanism breaks too much stuff. r=hiro

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae4b3c9f8ee5
Move scroll event declarations to nsGfxScrollFrame.cpp r=hiro

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Alexandru Michis [:malexandru]]

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

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

Comment 23

[Emilio Cobos Álvarez [:emilio]]

Yes, let's do that.

Comment 24

[Emilio Cobos Álvarez [:emilio]]

Should be fixed by backout.