Closed Bug 1358815 Opened 5 years ago Closed 4 years ago

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


(Toolkit :: Find Toolbar, defect, P4)




Tracking Status
firefox61 --- fixed


(Reporter: rjward0, Assigned: Gijs)


(Blocks 1 open bug)


(Keywords: perf, Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p1])


(2 files)

Here's the stack:

promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
Flags: qe-verify?
Priority: -- → P2
(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.

> @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 =;
      if (win.gFindBar) {

I wonder if this could use requestAnimationFrame instead of the forced flush.
Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Flags: qe-verify? → qe-verify-
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Priority: P3 → P4
Keywords: perf
This still exists. I'm tentatively setting this as a qf:p1 for now.
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][fxperf]
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?
Flags: needinfo?(mdeboer)
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p2]
(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.
Flags: needinfo?(mdeboer)
Assignee: nobody → gijskruitbosch+bugs
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p2] → [ohnoreflow][qf:f61][qf:p1][fxperf:p1]
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:

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...
Flags: needinfo?(bdahl)
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.
Blocks: 1371523
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].

Flags: needinfo?(bdahl)
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 on attachment 8958399 [details]
Bug 1358815 - update pdfjs for async-created findbar,

::: 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.
Attachment #8958399 - Flags: review?(bdahl) → review+
Attachment #8958237 - Flags: review?(mdeboer) → review?(jaws)
Comment on attachment 8958237 [details]
Bug 1358815 - remove sync reflow from find bar initialization,

::: 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.
Attachment #8958237 - Flags: review?(jaws) → review+
Pushed by
remove sync reflow from find bar initialization, r=jaws
update pdfjs for async-created findbar, r=bdahl
(In reply to Cristian Brindusan [:cristian_brindusan] from comment #17)
> Backed out for bc failures on browser_pdfjs_main.js.
> 02843ed01924ac509010a9aa215f40e31deafcaf
> Push with failures:
> 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:
> 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.
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by
remove sync reflow from find bar initialization, r=jaws
update pdfjs for async-created findbar, r=bdahl
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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).
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1450369
I'm removing the firefox extension code from github so we don't need to backport.
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.