Closed Bug 1071638 Opened 5 years ago Closed 5 years ago

Add test to ensure there are no uninterruptible reflows when loading about:newtab

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.2

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file, 2 obsolete files)

With bug 1071635 fixed we should add a test to ensure we don't regress this.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
The test seems to work as it currently shows the two reflows we want to get rid of:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js | unexpected uninterruptible reflow 'gPage.onPageFirstVisible/checkSizing/<@chrome://browser/content/newtab/newTab.js:506:1|' -
Stack trace:
chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js:runTests/<:23
chrome://mochitests/content/browser/browser/base/content/test/newtab/content-reflows.js:reflow:15
chrome://browser/content/newtab/newTab.js:gPage.onPageFirstVisible/checkSizing/<:506
null:null:0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js | unexpected uninterruptible reflow 'gPage.onPageFirstVisible/checkSizing/<@chrome://browser/content/newtab/newTab.js:506:1|' -
Stack trace:
chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_reflow_load.js:runTests/<:23
chrome://mochitests/content/browser/browser/base/content/test/newtab/content-reflows.js:reflow:15
chrome://browser/content/newtab/newTab.js:gPage.onPageFirstVisible/checkSizing/<:506
null:null:0
Attachment #8493815 - Attachment is obsolete: true
Attachment #8493853 - Flags: review?(adw)
Comment on attachment 8493853 [details] [diff] [review]
0001-Bug-1071638-Add-test-to-ensure-there-are-no-uninterr.patch, v2

Review of attachment 8493853 [details] [diff] [review]:
-----------------------------------------------------------------

Honest question: what are uninterruptible reflows and why are they bad?

::: browser/base/content/test/newtab/browser.ini
@@ +1,5 @@
>  [DEFAULT]
>  skip-if = buildapp == 'mulet' || e10s # Bug ?????? - about:newtab tests don't work in e10s
>  support-files =
>    head.js
> +  content-reflows.js

Nit: I prefer to add support-files inline with the test, keeping only files needed by multiple tests here at the top.

::: browser/base/content/test/newtab/browser_newtab_reflow_load.js
@@ +9,5 @@
> +/*
> + * Ensure that loading about:newtab doesn't cause uninterruptible reflows.
> + */
> +function* runTests() {
> +  Services.prefs.setBoolPref(PREF_INTRO_SHOWN, true);

The testing profile already sets this to true: http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#186

@@ +24,5 @@
> +  });
> +
> +  browser.loadURI("about:newtab");
> +  yield whenBrowserLoaded(browser);
> +  gBrowser.removeCurrentTab({animate: false});

So if an uninterruptible reflow happens, it will always happen before this point?

::: browser/base/content/test/newtab/content-reflows.js
@@ +9,5 @@
> +
> +  docShell.addWeakReflowObserver({
> +    reflow() {
> +      // Gather information about the current code path.
> +      let path = (new Error().stack).split("\n").slice(1).join("|");

Nit: Why not keep the line feeds?  Should be easier to read.
Attachment #8493853 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #4)
> Honest question: what are uninterruptible reflows and why are they bad?

There are two types of reflows. The "bad type" is a reflow that has to happen now or the current operation can't guarantee to return the correct result, e.g. retrieving document.body.offsetWidth. The good reflows are the ones that can happen whenever the layout engine thinks it's best to do them. Another good thing about the second type is that they are "interruptible" which means we can interrupt heavier ones and process some events in between to keep the browser responsive.

> Nit: I prefer to add support-files inline with the test, keeping only files
> needed by multiple tests here at the top.

Yeah, initially thought I'd write a second test that uses this but turns out the test didn't make sense.

> The testing profile already sets this to true:
> http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.
> js#186

Ah, thanks.

> > +  browser.loadURI("about:newtab");
> > +  yield whenBrowserLoaded(browser);
> > +  gBrowser.removeCurrentTab({animate: false});
> 
> So if an uninterruptible reflow happens, it will always happen before this
> point?

Yeah, I checked that with the current code the test fails. There might be some more reflows after the tab has loaded already but they're not as critical to user experience here. I thought about adding a ~2s wait before checking to ensure that no one optimizes for that and just delays reflows by a bit. WDYT?

> > +      // Gather information about the current code path.
> > +      let path = (new Error().stack).split("\n").slice(1).join("|");
> 
> Nit: Why not keep the line feeds?  Should be easier to read.

Yeah, good idea. Stole that from another test where it actually made sense to replace those.
Thanks for the explanation.

(In reply to Tim Taubert [:ttaubert] from comment #5)
> Yeah, I checked that with the current code the test fails. There might be
> some more reflows after the tab has loaded already but they're not as
> critical to user experience here. I thought about adding a ~2s wait before
> checking to ensure that no one optimizes for that and just delays reflows by
> a bit. WDYT?

Is the tab animation done by that point?  If so, then I'd say don't worry about it.  If not, then it might be good to keep listening until it's done.
Iteration: --- → 35.2
Points: --- → 3
Flags: firefox-backlog+
QA Whiteboard: [qa-]
QA Whiteboard: [qa-]
Flags: qe-verify-
https://hg.mozilla.org/mozilla-central/rev/77ae1deb677a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.