1358815 - 0.90ms uninterruptible reflow at getFindBar@chrome://browser/content/tabbrowser.xml:178:11

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P4)

Description

[Hazel Ward]

Here's the stack:

getFindBar@chrome://browser/content/tabbrowser.xml:178:11
@chrome://browser/content/browser.js:244:10
onCommand@resource:///modules/CustomizableWidgets.jsm:575:1
handleWidgetCommand@resource:///modules/CustomizableUI.jsm:1479:11
EventListener.handleEvent*buildWidget@resource:///modules/CustomizableUI.jsm:1374:7
getWidgetNode@resource:///modules/CustomizableUI.jsm:887:16
buildArea@resource:///modules/CustomizableUI.jsm:711:32
registerMenuPanel@resource:///modules/CustomizableUI.jsm:914:5
registerMenuPanel@resource:///modules/CustomizableUI.jsm:3051:5
ensureReady/this._readyPromise<@chrome://browser/content/customizableui/panelUI.js:383:11
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
TaskImpl@resource://gre/modules/Task.jsm:277:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
ensureReady@chrome://browser/content/customizableui/panelUI.js:343:26
show/<@chrome://browser/content/customizableui/panelUI.js:153:7
Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
show@chrome://browser/content/customizableui/panelUI.js:152:12
toggle@chrome://browser/content/customizableui/panelUI.js:140:7
handleEvent@chrome://browser/content/customizableui/panelUI.js:296:11
EventListener.handleEvent*init@chrome://browser/content/customizableui/panelUI.js:57:5
_delayedStartup@chrome://browser/content/browser.js:1396:5
EventListener.handleEvent*onLoad@chrome://browser/content/browser.js:1239:5
onload@chrome://browser/content/browser.xul:1:1

Comment 1

[Florian Quèze [:florian]]

(In reply to Robert Ward from comment #0)

> getFindBar@chrome://browser/content/tabbrowser.xml:178:11
          // Force a style flush to ensure that our binding is attached.
          findBar.clientTop;

> @chrome://browser/content/browser.js:244:10
this.__defineGetter__("gFindBar", function() {
  return window.gBrowser.getFindBar();
});

> onCommand@resource:///modules/CustomizableWidgets.jsm:575:1
This is:
    id: "find-button",
    shortcutId: "key_find",
    tooltiptext: "find-button.tooltiptext3",
    defaultArea: CustomizableUI.AREA_PANEL,
    onCommand(aEvent) {
      let win = aEvent.target.ownerGlobal;
      if (win.gFindBar) {
        win.gFindBar.onFindCommand();
      }
    }

I wonder if this could use requestAnimationFrame instead of the forced flush.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

This still exists. I'm tentatively setting this as a qf:p1 for now.

Comment 3

[:Gijs (he/him)]

Mirroring the qf:p1 into fxperf as p2 (not sure we can fix for 60, though I'd kind of like to) because this is technically frontend code. Mike, can you answer whether Florian's suggestion (comment #1) to use rAF here would help?

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #3)
> Mike, can you answer whether Florian's suggestion (comment #1) to use rAF
> here would help?

I think Florian is right there; it'd be good to try rAf here.

Comment 5

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a693fd3ae3575d14b86961c6274a211e6449c190

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1358815 - remove sync reflow from find bar initialization,

This removes the sync reflow from almost all cases. The only case where we keep it is when a keypress
caught in content triggers a sync message to the parent process. We should clean this up in bug 1371523.

I've tried to fix the tests, but a lot of them seem to be disabled anyway...

Review commit: https://reviewboard.mozilla.org/r/227184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227184/

Comment 7

[:Gijs (he/him)]

So the TL;DR for these patches is that `getFindBar` becomes async, and `gFindBar` and `getCachedFindBar` are sync but may return null. (In fact, the async variant can also return null, but only if the window/tab goes away mid-way.)

This seems to generally work OK. I think I got to most test issues by now.

The one issue I didn't solve, and for which the sync reflow will persist (for now) is where a key is used to trigger FAYT. This currently uses sync ipc, and I'd like to tackle that next, in the bug for the sync IPC stuff, at which point I think we can also more easily make that 1 callsite not expect a find bar binding synchronously.

Rather late in my patch work here, I noticed PdfStreamConverter.jsm and PdfjsChromeUtils.jsm try to use `getFindBar` as well. I have to admit I'm a bit confused by this code, and how to go about making it OK with the idea that getting the find bar may not be sync. Brendan, can you help enlighten me as to what the code at:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#66
https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#348
and
https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm#185

is supposed to do, and how best to teach it that stuff might now be async? It seems to mostly just want to add listeners, and it kind of feels like that's using the wrong push/pull stuff anyway (ie rather than forcing creation and initialization of the findbar, it should wait for the find bar to be created before adding any listeners). But maybe I misunderstand what that code is trying to do...

Comment 8

[:Gijs (he/him)]

Note that a few of the test changes are in tests that are actually skip-if = true right now, and indeed they don't pass either with or without my changes, but I did my best to make it easier for whoever is going to try to re-enable those tests to do so without having to also fight this change.

Comment 9

[Brendan Dahl [:bdahl]]

PDFJS needs to know if the integrated firefox findbar is supported so it can decide to show/hide it's built in findbar if it's within a frame.

Some of this code is overly complicated since we left some of it to support older versions of Firefox for the extension. I think we're at the point now where we can get rid of the check for the new find events[1]. 

Then I think we'll just have to handle the async/null case here[2].

[1] https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#347
[2] https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm#190

Comment 10

[:Gijs (he/him)]

OK, I think I've sorted things out more or less in accordance with comment #9, review request incoming. I'll be happy to put up a github PR afterwards as well, assuming it's OK to do it this way around. :-)

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1358815 - update pdfjs for async-created findbar,

Review commit: https://reviewboard.mozilla.org/r/227368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227368/

Comment 12

[Brendan Dahl [:bdahl]]

Comment on attachment 8958399 [details]
Bug 1358815 - update pdfjs for async-created findbar,

https://reviewboard.mozilla.org/r/227368/#review233236

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
(Diff revision 1)
> -                  .QueryInterface(Ci.nsIDocShell)
> -                  .chromeEventHandler;
> -}
> -
> -function getFindBar(domWindow) {
> -  if (PdfjsContentUtils.isRemote) {

We can get rid of PdfjsContentUtils.isRemote now.

Comment 13

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

Comment on attachment 8958237 [details]
Bug 1358815 - remove sync reflow from find bar initialization,

https://reviewboard.mozilla.org/r/227184/#review233556

::: browser/base/content/test/general/browser_bug537013.js:104
(Diff revision 1)
>    gBrowser.selectedTab = tabs[0];
>    ok(gFindBar.hidden, "First tab doesn't show find bar!");
>  
>    // Set up a third tab, no tests here
>    gBrowser.selectedTab = tabs[2];
> +  gBrowser.selectedTab.addEventListener("TabFindInitialized", continueTests4);

add {once: true} here and then remove the removeEventListener call below.

::: toolkit/content/tests/browser/browser_bug1198465.js:31
(Diff revision 1)
> +  // hasn't been resolved yet so we're not accidentally not really testing what
> +  // we want to test.

The dual negatives here are a bit confusing.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8958237 [details]
Bug 1358815 - remove sync reflow from find bar initialization,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227184/diff/1-2/

Comment 15

[:Gijs (he/him)]

Comment on attachment 8958399 [details]
Bug 1358815 - update pdfjs for async-created findbar,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227368/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2a0a0990e1a
remove sync reflow from find bar initialization, r=jaws
https://hg.mozilla.org/integration/autoland/rev/880736d97de3
update pdfjs for async-created findbar, r=bdahl

Comment 17

[Cristian Brindusan [:cbrindusan]]

Backed out for bc failures on browser_pdfjs_main.js.

https://hg.mozilla.org/integration/autoland/rev/02843ed01924ac509010a9aa215f40e31deafcaf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=880736d97de3f973f545abdbcc993cd4288f217c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168297353&repo=autoland&lineNumber=5027

Comment 18

[:Gijs (he/him)]

(In reply to Cristian Brindusan [:cristian_brindusan] from comment #17)
> Backed out for bc failures on browser_pdfjs_main.js.
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 02843ed01924ac509010a9aa215f40e31deafcaf
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=880736d97de3f973f545abdbcc993cd4288f217c&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=success&filter-
> resultStatus=pending&filter-resultStatus=running 
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=168297353&repo=autoland&lineNumber=5027

Huh. I don't understand why this doesn't fail anywhere else. Regardless, that test assertion is bogus with the pdfjs refactor in this bug, so I'm just going to remove it.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8958399 [details]
Bug 1358815 - update pdfjs for async-created findbar,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227368/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b819da45df6
remove sync reflow from find bar initialization, r=jaws
https://hg.mozilla.org/integration/autoland/rev/342092ab84c4
update pdfjs for async-created findbar, r=bdahl

Comment 21

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/4b819da45df6
https://hg.mozilla.org/mozilla-central/rev/342092ab84c4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This change needs to be backported to the upstream pdf.js repo so it doesn't
get clobbered on the next update (like I almost just did).
https://github.com/mozilla/pdf.js

Comment 23

[Brendan Dahl [:bdahl]]

I'm removing the firefox extension code from github so we don't need to backport.

https://github.com/mozilla/pdf.js/pull/9566