1429824 - Scroll to bottom feature forces many reflows and slow down the netmonitor

Status: RESOLVED DUPLICATE of bug 1606183

(DevTools :: Netmonitor, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

We discussed at length in bug 1416714 the cost of automatic scroll to bottom feature in netmonitor. Profiles show that isScrolledToBottom method is slow and called a lot. bug 1416714 comment 15 describes potential ways to optimize this.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1429824 - Prevent reflows related to scroll to bottom feature.

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

Comment 2

[Alexandre Poirot [:ochameau]]

The challenge with Julien's idea (bug 1416714 comment 15) is that after all it only moves the reflow/call to isScrolledToBottom from componentWillUpdate to scroll event processing. 
That's because scroll events also fire when we append a new request/line. So that doesn't reduce the number of reflows.
Also, having the reflows done from componentWillUpdate or within scroll event listener doesn't seem to make any difference in term of perf.
It would be improving things if we can somehow make the difference between a user scroll and a DOM change.
But most of the attributes of scroll event object are unset... making it hard to filter the events.
In this patch I set a flag on componentWillUpdate, but this is weak and subject to races.
We could use `wheel` event instead. This one is fired only on user interaction and comes with a deltaY attribute. This attribute helps calling isScrolledToBottom only when it is strictly necessary and reduces even more the number of reflows during user scroll!
But this event doesn't fire when the user grabs the scrollbar and so we still have to listen for scroll event...

Comment 3

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c7a68fdb0183368932d909dd91e821cc52c1b9b2&newProject=try&newRevision=1eb737d4f2fa1134570b4af48894e67753dcb9c3&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
DAMP reports a 4% improvement on complicated.requestsFinished.
That sounds great. Last try from bug 1416714 comment 7 reported a 3% win when disabling scroll-to-bottom completely.
So it looks like, with such patch, this feature doesn't impact performance significantly anymore.

Now, as I said, there is a possible races where a user scroll may be ignored.
So that it may keep scrolling to top even if user started scrolling.
Also, it may regress performances when scrolling.

It would be great to have this patch tested by various people to see if these issues are real or not.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

Julien, Jan, Could you give this patch a try and tell me if you see anything suspicious? Big newspaper websites like cnn.com/lemonde.fr are good tests as they keep dispatching requests for a while!

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c7a68fdb0183368932d909dd91e821cc52c1b9b2&newProject=try&newRevision=da4044279a633e2ed69a2e70fc3de63f6a96463f&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
Here is a try run that completely disable scroll to bottom.
We get a 4.33% win, compared to 4.16% with the current patch, so it looks pretty good regarding netmonitor performance on reload. Now, again, scroll performance may have regressed and DAMP doesn't track that.

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

https://reviewboard.mozilla.org/r/212096/#review218102

::: devtools/client/netmonitor/src/components/RequestListContent.js:190
(Diff revision 1)
>    }
>  
>    /**
>     * Scroll listener for the requests menu view.
>     */
> -  onScroll() {
> +  onScroll(event) {

nit: event is unused, please remove it.

::: devtools/client/netmonitor/src/components/RequestListContent.js:199
(Diff revision 1)
> +    // To prevent slow reflows done in isScrolledToBottom.
> +    if (this.ignoreNextScroll) {
> +      this.ignoreNextScroll = false;
> +      return;
> +    }
> +    this.scrolledManually = !this.isScrolledToBottom();

Nice trick! Reducing isScrolledToBottom() can decrease reflow accordingly.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

https://reviewboard.mozilla.org/r/212096/#review218160

From my tests it seems to work fine.

Here are some observations, I tested with various heavy sites like lemonde (like you suggested) or thedailybeast.com which are request-heavy:

1. Thanks to Async Pan Zoom (APZ) scrolling the netmonitor list stays smooth even if we have sync reflow -- but when the list is bigger we do see some white checkerboarding while the reflow happens. I believe this is fine as I don't think users are scrolling frenetically while looking at this list.
2. Scrolling the page itself with the scrollbar with the netmonitor opened seems to suffer from the reflows, even async ones. At first the scroll stays smooth but when a lot of requests are in the table we can see big lags. I believe this can be different when we scroll using the mousewheel or the trackpad, or on different OS (I'm on Linux) because when async pan zoom is used I think we don't see these lags at all; so YMMV.
But I think this is worse without your patch.

::: devtools/client/netmonitor/src/components/RequestListContent.js:99
(Diff revision 1)
>        interactive: true
>      });
> +    // Set a flag on "scroll" event, used in componentWillUpdate, in order to help
> +    // knowing if we should keep automatically scrolling to bottom without doing
> +    // too many reflows.
> +    this.scrolledManually = false;

I don't know this code, so it will just be a not of advice: you'll need to reset this flag when we clear the view (either when the user does it or when we reload the page for example). I don't know if we get a new mounted component in that case ?

::: devtools/client/netmonitor/src/components/RequestListContent.js:109
(Diff revision 1)
>    componentWillUpdate(nextProps) {
>      // Check if the list is scrolled to bottom before the UI update.
>      // The scroll is ever needed only if new rows are added to the list.
>      const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size;
> -    this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom();
> +    this.shouldScrollBottom = delta > 0 && !this.scrolledManually;
> +    this.ignoreNextScroll = delta > 0;

nit: we don't use the delta value, so maybe just keep a boolean like this:
```
const hasNewRows = nextProps.displayedRequests.size > this.props.displayedRequests.size;
```

another nit is: why don't you do this in `componentDidUpdate`

::: devtools/client/netmonitor/src/components/RequestListContent.js:117
(Diff revision 1)
>    componentDidUpdate(prevProps) {
>      let node = this.refs.contentEl;
>      // Keep the list scrolled to bottom if a new row was added
> -    if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) {
> +    if (this.shouldScrollBottom) {
>        // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow.
>        node.scrollTop = MAX_SCROLL_HEIGHT;

have you tried using `scrollIntoView` yet?

::: devtools/client/netmonitor/src/components/RequestListContent.js:199
(Diff revision 1)
> +    // To prevent slow reflows done in isScrolledToBottom.
> +    if (this.ignoreNextScroll) {
> +      this.ignoreNextScroll = false;
> +      return;
> +    }
> +    this.scrolledManually = !this.isScrolledToBottom();

One thing I've seen in some react code (in componentDidUpdate mostly but this can apply in other places) is to wrap things that might trigger sync reflow with requestAnimationFrame, so that a reflow should happen asynchronously.

eg:
```
onScroll() {
  ...
  requestAnimationFrame(() => {
    this.scrolledManually = !this.isScrolledToBottom();
  });
}
```

Comment 8

[Julien Wajsberg [:julienw]]

To answer this from a previous comment:

> The challenge with Julien's idea (bug 1416714 comment 15) is that after all it only moves the reflow/call to isScrolledToBottom from componentWillUpdate to scroll event processing.

Scrolling is made smooth thanks to APZ so reflows are much less visible. Moreover a user is unlikely to scroll much while a page he's analyzing is loading -- he can scroll but not frenetically.

Lastly we must throttle scroll events (for example with requestAnimationFrame).
(Note: I confusedly thought React does this if we use React's event mechanism, but I don't see anything in the doc about this. And anyway we don't use React for the scroll event (does somebody know why?)).

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8941879 [details]
> Bug 1429824 - Prevent reflows related to scroll to bottom feature.
> 
> Julien, Jan, Could you give this patch a try and tell me if you see anything
> suspicious? Big newspaper websites like cnn.com/lemonde.fr are good tests as
> they keep dispatching requests for a while!

I was testing both with/without the patch on cnn.com and thedailybeast.com
I am not seeing and wrong behavior and the scrolling (perceived) performance
seems to be a bit better (not worse).

Sometimes is hard to scroll to bottom (and activate the 'scroll to bottom' feature).
After releasing the scroll-thumb the content doesn't stick at the bottom.
But, I was experiencing this even before (without the patch).

F+, I don't see any reason to block this.

Thanks,
Honza

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Comment on attachment 8941879 [details]
> Bug 1429824 - Prevent reflows related to scroll to bottom feature.
> 
> https://reviewboard.mozilla.org/r/212096/#review218160
> 
> From my tests it seems to work fine.
> 
> Here are some observations, I tested with various heavy sites like lemonde
> (like you suggested) or thedailybeast.com which are request-heavy:
> 
> 1. Thanks to Async Pan Zoom (APZ) scrolling the netmonitor list stays smooth
> even if we have sync reflow -- but when the list is bigger we do see some
> white checkerboarding while the reflow happens. I believe this is fine as I
> don't think users are scrolling frenetically while looking at this list.

You keep thinking you are on FxOS with content processes and APZ, but we are not ;)
netmonitor runs in chrome, in parent processes and I don't think we have APZ there.

> ::: devtools/client/netmonitor/src/components/RequestListContent.js:99
> (Diff revision 1)
> >        interactive: true
> >      });
> > +    // Set a flag on "scroll" event, used in componentWillUpdate, in order to help
> > +    // knowing if we should keep automatically scrolling to bottom without doing
> > +    // too many reflows.
> > +    this.scrolledManually = false;
> 
> I don't know this code, so it will just be a not of advice: you'll need to
> reset this flag when we clear the view (either when the user does it or when
> we reload the page for example). I don't know if we get a new mounted
> component in that case ?

componentDidMount is called everytime the request list is emptied.

> ::: devtools/client/netmonitor/src/components/RequestListContent.js:109
> (Diff revision 1)
> >    componentWillUpdate(nextProps) {
> >      // Check if the list is scrolled to bottom before the UI update.
> >      // The scroll is ever needed only if new rows are added to the list.
> >      const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size;
> > -    this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom();
> > +    this.shouldScrollBottom = delta > 0 && !this.scrolledManually;
> > +    this.ignoreNextScroll = delta > 0;
> 
> nit: we don't use the delta value, so maybe just keep a boolean like this:
> ```
> const hasNewRows = nextProps.displayedRequests.size >
> this.props.displayedRequests.size;
> ```
> 
> another nit is: why don't you do this in `componentDidUpdate`

We don't have `nextProps` in componentDidUpdate, so we can't compute this `delta` thing.

> 
> ::: devtools/client/netmonitor/src/components/RequestListContent.js:117
> (Diff revision 1)
> >    componentDidUpdate(prevProps) {
> >      let node = this.refs.contentEl;
> >      // Keep the list scrolled to bottom if a new row was added
> > -    if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) {
> > +    if (this.shouldScrollBottom) {
> >        // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow.
> >        node.scrollTop = MAX_SCROLL_HEIGHT;
> 
> have you tried using `scrollIntoView` yet?

No, but this in independant of this bug/patch.
Here, I would like to focus on the code that do reflows just to compute *if* we should scroll to bottom.
We should look into that independantly. See if that make any difference.

> ::: devtools/client/netmonitor/src/components/RequestListContent.js:199
> (Diff revision 1)
> > +    // To prevent slow reflows done in isScrolledToBottom.
> > +    if (this.ignoreNextScroll) {
> > +      this.ignoreNextScroll = false;
> > +      return;
> > +    }
> > +    this.scrolledManually = !this.isScrolledToBottom();
> 
> One thing I've seen in some react code (in componentDidUpdate mostly but
> this can apply in other places) is to wrap things that might trigger sync
> reflow with requestAnimationFrame, so that a reflow should happen
> asynchronously.
> 
> eg:
> ```
> onScroll() {
>   ...
>   requestAnimationFrame(() => {
>     this.scrolledManually = !this.isScrolledToBottom();
>   });
> }
> ```

Unfortunately here, it is very important to do it synchronously :/
Otherwise it increase the existing race in this patch:
* onScroll will fire for a user event scrolling up, we should stop immediatly scrolling to bottom
* instead, of setting scrolledManually to false immediately, it is postpone to later. Instead, a new request slices in. scrolledManually is still false, we do scroll to bottom
* the animation frame is over, we try updating scrolledManually, but as the new request forced scrolling to bottom, scrolledManually stays false. => We ignored used event.

This scenario is the typical race this patch may introduce.
Now, may be this race never happens thanks to reflow being synchronous, but I wouldn't be surprised if for some reason, or only on some platform, reflow event is slightly different and introduces races.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

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

Comment 12

[Julien Wajsberg [:julienw]]

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> (In reply to Julien Wajsberg [:julienw] from comment #7)
> > 
> > 1. Thanks to Async Pan Zoom (APZ) scrolling the netmonitor list stays smooth
> > even if we have sync reflow -- but when the list is bigger we do see some
> > white checkerboarding while the reflow happens. I believe this is fine as I
> > don't think users are scrolling frenetically while looking at this list.
> 
> You keep thinking you are on FxOS with content processes and APZ, but we are
> not ;)
> netmonitor runs in chrome, in parent processes and I don't think we have APZ
> there.

I'm not thinking about FxOS at all :) I see that the scroll stays smooth, maybe it isn't APZ but this is what I see, on Linux at least :) (and I think Linux is the worst platform for smoothness, as a lot more effort is done on Mac and Windows).

> 
> > ::: devtools/client/netmonitor/src/components/RequestListContent.js:109
> > (Diff revision 1)
> > >    componentWillUpdate(nextProps) {
> > >      // Check if the list is scrolled to bottom before the UI update.
> > >      // The scroll is ever needed only if new rows are added to the list.
> > >      const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size;
> > > -    this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom();
> > > +    this.shouldScrollBottom = delta > 0 && !this.scrolledManually;
> > > +    this.ignoreNextScroll = delta > 0;
> > 
> > nit: we don't use the delta value, so maybe just keep a boolean like this:
> > ```
> > const hasNewRows = nextProps.displayedRequests.size >
> > this.props.displayedRequests.size;
> > ```
> > 
> > another nit is: why don't you do this in `componentDidUpdate`
> 
> We don't have `nextProps` in componentDidUpdate, so we can't compute this
> `delta` thing.

componentWillUpdate happens before the update: the parameter is "nextProps", this.props are the prevProps.
componentDidUpdate happens after the update: the parameter is "prevProps", this.props are the nextProps.

So you _can_ compute the delta :) and this would make the patch simpler.

> 
> Unfortunately here, it is very important to do it synchronously :/
> Otherwise it increase the existing race in this patch:
> * onScroll will fire for a user event scrolling up, we should stop
> immediatly scrolling to bottom
> * instead, of setting scrolledManually to false immediately, it is postpone
> to later. Instead, a new request slices in. scrolledManually is still false,
> we do scroll to bottom

=> maybe this request to scroll could be in a requestAnimationFrame too.
But this isn't in the scope for this patch so let's look at this elsewhere.

> * the animation frame is over, we try updating scrolledManually, but as the
> new request forced scrolling to bottom, scrolledManually stays false. => We
> ignored used event.
> 
> This scenario is the typical race this patch may introduce.
> Now, may be this race never happens thanks to reflow being synchronous, but
> I wouldn't be surprised if for some reason, or only on some platform, reflow
> event is slightly different and introduces races.

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> ::: devtools/client/netmonitor/src/components/RequestListContent.js:117
> >        node.scrollTop = MAX_SCROLL_HEIGHT;
> 
> have you tried using `scrollIntoView` yet?

DAMP report no win:
  https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=935966d36f6cc7146c2d99de638c4b68f43d9fc2&newProject=try&newRevision=ac1a7cb674832bba4fa0ef007f1a8a2270ce32cd&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1

Nor does the profiler. It is interesting to see that I get the very expect same duration for both!

scrollIntoView:
  https://perfht.ml/2BeKAyI
  360ms
scrollTop:
  https://perfht.ml/2mRNULL
  360ms

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

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

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

https://reviewboard.mozilla.org/r/212096/#review220530

Thanks for the patch Alex.
R+ assuming try is green

Honza

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b764ca8d82da
Prevent reflows related to scroll to bottom feature. r=Honza

Comment 17

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome devtools at devtools/client/netmonitor/test/browser_net_autoscroll.js on a CLOSED TREE

Push for which the failing jobs ran: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a678eec13c6e6bf1662cc4db5719bbb413c63f6

recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158014531&repo=autoland&lineNumber=3988

Backout: https://hg.mozilla.org/integration/autoland/rev/e8d981d2ce37ecf6f34909ed3bd76dd536012ef3

Comment 18

[Alexandre Poirot [:ochameau]]

This patch made browser_net_autoscroll.js be intermittently failing.

Here is a new patch that simplifies a bit the various variables used around scroll-to-bottom.
(renamed scrolledManually to shouldScrollToBottom)

It fixed the disabling of scroll to bottom when selecting a request line (it was broken and kept scrolling down)
and fixed the race that made this test fail by setting ignoreNextScroll only when it is strictly needed (i.e. when we set scrollTop to MAX_SCROLL_HEIGHT).
I added some more comments!


Here is a handy test document to verify all this:
data:text/html,<script>window.setInterval(()=>{var%20x=new%20XMLHttpRequest();%20x.open("GET",%20"http://mozilla.org",%20false);%20x.send(null);},%20250)</script>

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

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

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

Jan, Could you look at the latest version?

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

https://reviewboard.mozilla.org/r/212096/#review221260

Looks good, but nees rebase on mc.

I am seeing:

Hunk #1 FAILED at 87
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/RequestListContent.js.rej

Honza

Comment 22

[Alexandre Poirot [:ochameau]]

Looks like it still fails, but only when stylo is disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73753e3ed0082ac6369223b5199f9b1b24a15e19&selectedJob=158640464
https://treeherder.mozilla.org/logviewer.html#?job_id=158640464&repo=try&lineNumber=7527

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212096/diff/4-5/

Comment 24

[Alexandre Poirot [:ochameau]]

Rebased and re run it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a5ccad30f6709e8733b06e5b389a454229f3124

It highlights that it is intermittent and isn't specific to stylo being disabled as it occured on linux64-debug.

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1429824 - Always use a valid transformation against the waterwall.

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

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1429824 - Try to prevent races in browser_net_autoscroll.js.

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

Comment 27

[Alexandre Poirot [:ochameau]]

Comment on attachment 8941879 [details]
Bug 1429824 - Prevent reflows related to scroll to bottom feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212096/diff/5-6/

Comment 28

[Alexandre Poirot [:ochameau]]

Comment on attachment 8950565 [details]
Bug 1429824 - Always use a valid transformation against the waterwall.

Without this patch, you get a tons of these error in stdout:
Console message: [JavaScript Warning: "Error in parsing value for ‘transform’ after substituting variables. Generated value was ‘ scaleX(Infinity)’.  Falling back to ‘initial’." {file: "chrome://devtools/content/netmonitor/src/assets/styles/RequestList.css" line: 606 column: 12267 source: " scaleX(Infinity)"}]

This doesn't help fixing the intermittent, but cleans up the test output!

Comment 29

[Alexandre Poirot [:ochameau]]

Comment on attachment 8950566 [details]
Bug 1429824 - Try to prevent races in browser_net_autoscroll.js.

But this patch seems to fix the intermittent against browser_net_autoscroll.js!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e7672796b003b348b3e6de71aa45e8972b38f13&selectedJob=161306276

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8950565 [details]
Bug 1429824 - Always use a valid transformation against the waterwall.

https://reviewboard.mozilla.org/r/219840/#review225942

Looks reasonable, thanks.
R+
Honza

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8950566 [details]
Bug 1429824 - Try to prevent races in browser_net_autoscroll.js.

https://reviewboard.mozilla.org/r/219842/#review225944

Ok, let's see if this helps, R+

Honza

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf796a41ff7d
Prevent reflows related to scroll to bottom feature. r=Honza
https://hg.mozilla.org/integration/autoland/rev/e3d4ec2c18b5
Always use a valid transformation against the waterwall. r=Honza
https://hg.mozilla.org/integration/autoland/rev/500836846f62
Try to prevent races in browser_net_autoscroll.js. r=Honza

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cf796a41ff7d
https://hg.mozilla.org/mozilla-central/rev/e3d4ec2c18b5
https://hg.mozilla.org/mozilla-central/rev/500836846f62

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/994a8d6eccbc
Backed out 3 changesets for frequently failing  devtools/client/netmonitor/test/browser_net_autoscroll.js (bug 1438412) a=backout

Comment 35

[Eliza Balazs [:ebalazs_]]

Backed out 3 changesets (bug 1429824) for frequently failing devtools/client/netmonitor/test/browser_net_autoscroll.js (bug 1438412) a=backout

push with failures:https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=9b69cc60e5848f2f8802c911fd00771b50eed41f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=162383724

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162383724&repo=mozilla-central&lineNumber=2158

backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=994a8d6eccbcdc6106794705bd77e3ac5f031be2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

Comment 36

[:Harald Kirschner :digitarald]

Fixed by bug 1606183 using scroll anchoring.

FWIW, other things still force reflows but I have my doubts that those reflows are bad just because they are forced by accessing DOM sizing information – layout has to happen at some point after adding elements.