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

RESOLVED FIXED in Firefox 61

Status

()

defect
P4
normal
RESOLVED FIXED
2 years ago
a year 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
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
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]

Updated

2 years ago
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]

Updated

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
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].

[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

a year 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

a year 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

a year ago
Attachment #8958237 - Flags: review?(mdeboer) → review?(jaws)
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

a year 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

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b819da45df6
https://hg.mozilla.org/mozilla-central/rev/342092ab84c4
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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).
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.