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

RESOLVED FIXED in Firefox 46

Status

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: philor, Assigned: jryans)

Tracking

({intermittent-failure})

unspecified
Firefox 47
intermittent-failure
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Updated

3 years ago
See Also: → bug 1143237
(Assignee)

Updated

3 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
:ntim, planning to adapt your patch with async RDM startup from bug 828008 to resolve this one, just so you are aware.
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
(Assignee)

Comment 4

3 years ago
Created attachment 8713840 [details]
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/
Attachment #8713840 - Flags: review?(poirot.alex)
(Assignee)

Comment 5

3 years ago
:ochameau, this ended up a lot larger than I had hoped... :/ There were just many places where the tests were quite racy.
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 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.
Attachment #8713840 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 8

3 years ago
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 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b884d7fa86f5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
This affects Aurora as well. Can we please request approval for uplift? Thanks!
status-firefox46: --- → affected
Flags: needinfo?(jryans)
(Assignee)

Comment 13

3 years ago
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
Flags: needinfo?(jryans)
Attachment #8713840 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Blocks: 1242205
(Assignee)

Updated

3 years ago
Blocks: 1227823
(Assignee)

Updated

3 years ago
Blocks: 1206337
(Assignee)

Updated

3 years ago
Blocks: 1222628
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.
Attachment #8713840 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(jryans)
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b90121826d2d
status-firefox46: affected → fixed
Flags: needinfo?(jryans)

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.