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

RESOLVED FIXED in Firefox 61

Status

()

P4
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: rjward0, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {perf})

unspecified
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p1])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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

Updated

2 years ago
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.
          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.
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
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]

Updated

11 months ago
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]

Updated

10 months ago
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]

Updated

8 months ago
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]
(Assignee)

Comment 3

8 months ago
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)

Updated

7 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p2] → [ohnoreflow][qf:f61][qf:p1][fxperf:p1]
Comment hidden (mozreview-request)
(Assignee)

Comment 7

7 months ago
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...
Flags: needinfo?(bdahl)
(Assignee)

Comment 8

7 months ago
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.
(Assignee)

Updated

7 months ago
Blocks: 1371523

Comment 9

7 months ago
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
Flags: needinfo?(bdahl)
(Assignee)

Comment 10

7 months ago
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 hidden (mozreview-request)

Comment 12

7 months ago
mozreview-review
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.
Attachment #8958399 - Flags: review?(bdahl) → review+
(Assignee)

Updated

7 months ago
Attachment #8958237 - Flags: review?(mdeboer) → review?(jaws)

Comment 13

7 months ago
mozreview-review
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.
Attachment #8958237 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

7 months ago
Pushed by gijskruitbosch@gmail.com:
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
(Assignee)

Comment 18

7 months ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 20

7 months ago
Pushed by gijskruitbosch@gmail.com:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b819da45df6
https://hg.mozilla.org/mozilla-central/rev/342092ab84c4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox61: --- → fixed
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).
https://github.com/mozilla/pdf.js
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1450369
I'm removing the firefox extension code from github so we don't need to backport.

https://github.com/mozilla/pdf.js/pull/9566
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.