1239562 - Intermittent browser_responsiveui.js | Floating scrollbars shouldn't change the width - Got 1280, expected 320

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect)

Description

[Phil Ringnalda (:philor)]

https://treeherder.mozilla.org/logviewer.html#?job_id=3070021&repo=mozilla-central

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ntim, planning to adapt your patch with async RDM startup from bug 828008 to resolve this one, just so you are aware.

Comment 2

[Intermittent Failures Robot]

8 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-central: 4
* try: 1
* mozilla-inbound: 1
* fx-team: 1
* b2g-inbound: 1

Platform breakdown:
* linux64: 4
* linux32: 3
* windows7-32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1239562&startday=2016-01-18&endday=2016-01-24&tree=all

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a8ee8db4c3a

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

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

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ochameau, this ended up a lot larger than I had hoped... :/ There were just many places where the tests were quite racy.

Comment 6

[Intermittent Failures Robot]

7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-central: 4
* fx-team: 2
* mozilla-inbound: 1

Platform breakdown:
* linux32: 4
* linux64: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1239562&startday=2016-01-25&endday=2016-01-31&tree=all

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8713840 [details]
MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

https://reviewboard.mozilla.org/r/32837/#review29789

Looks good overall, I just hope we can simplify the notifyResize thing.

::: devtools/client/responsivedesign/responsivedesign-child.js:28
(Diff revision 1)
> +  addMessageListener("ResponsiveMode:NotifyOnResize", notifyOnResize);

Per my previous comment, we could just get rid of this message (NotifyOnResize). It looks weird to have something else than just Start/Stop here. Even Stop could be done in startResponsiveMode.

::: devtools/client/responsivedesign/responsivedesign-child.js:62
(Diff revision 1)
> -  }, false);
> +    }, false);

To be perfect, we could removeEventListener this in stopResponsiveMode...
but it doesn't seem to pull much dependencies in its scope. So it doesn't looks very important at the first look.

::: devtools/client/responsivedesign/responsivedesign.jsm:207
(Diff revision 1)
> +    }

Is it the key part of this patch?
(with the related child.js change)

So, is it important to register the "resize" event listener in the child, before calling ResponsiveMode:Start/startResponsiveMode?
If yes, we should tweak the comment about that.

Otherwise, if not, which seems likely? What about passing another argument to ResponsideMode:Start?
Like requiresFloatingScrollbars. We could go for `testing` or `notifyOnResize`. I don't think we need another custom message just for this.

If that helps, you can delay resolving Start:Done after DOMWindowCreated and the resize registering.

::: devtools/client/responsivedesign/test/browser_responsive_cmd.js:27
(Diff revision 1)
> +          return helpers.setInput(options, "resize toggle");

I don't fully understand helpers API, but can't we resolve setup once `done` is resolve?
I imagine not, there is some action being done in between setup and post?

This `done` global seems weak. But I don't see how to do otherwise.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/32837/#review29789

> Per my previous comment, we could just get rid of this message (NotifyOnResize). It looks weird to have something else than just Start/Stop here. Even Stop could be done in startResponsiveMode.

Yep, the separate message is gone now.

> To be perfect, we could removeEventListener this in stopResponsiveMode...
> but it doesn't seem to pull much dependencies in its scope. So it doesn't looks very important at the first look.

At first I was worried there could be some resize event after the "Stop" message that would be missed... but now the close() method yields on the resize message from the last size change before sending "Stop" so it should be fine.

I am now removing the listener.

> Is it the key part of this patch?
> (with the related child.js change)
> 
> So, is it important to register the "resize" event listener in the child, before calling ResponsiveMode:Start/startResponsiveMode?
> If yes, we should tweak the comment about that.
> 
> Otherwise, if not, which seems likely? What about passing another argument to ResponsideMode:Start?
> Like requiresFloatingScrollbars. We could go for `testing` or `notifyOnResize`. I don't think we need another custom message just for this.
> 
> If that helps, you can delay resolving Start:Done after DOMWindowCreated and the resize registering.

I think the "key" part is that we are using yield to block the UI instance from setting the size until after the resize listener is bound.

Since we currently send the "Start" message right after, it seems fine to combine notifyOnResize into the start message.

It seems to work well locally, hopefully try will agree.

> I don't fully understand helpers API, but can't we resolve setup once `done` is resolve?
> I imagine not, there is some action being done in between setup and post?
> 
> This `done` global seems weak. But I don't see how to do otherwise.

I don't know the helpers API very well either...  Yes, there is work done after `setup` and before `post`: it runs the command.  So, I need to wait for the event before that happens and check for it after that.

Since the current helpers only give `setup` and `post` to work with, I think the `done` global is still needed.  But yeah, I agree it's not the best.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=699aa465ff93

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b884d7fa86f5

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b884d7fa86f5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This affects Aurora as well. Can we please request approval for uplift? Thanks!

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8713840 [details]
MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

Approval Request Comment
[Feature/regressing bug #]: No specific regression, race conditions in tests added over time
[User impact if declined]: No user impact, many pieces of responsive design tests to remove race conditions
[Describe test coverage new/current, TreeHerder]: On m-c, tests greatly improved (so far, at least!)
[Risks and why]: Low, while some product code changes are included, they are only meant to improve test behavior.
[String/UUID change made/needed]: None

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8713840 [details]
MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

Eliminates some race conditions in responsive design tests.
Please uplift to aurora.

Comment 15

[Carsten Book [:Tomcat]]

has problems during uplift:

warning: conflicts while merging devtools/client/responsivedesign/responsivedesign-child.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b90121826d2d