Intermittent browser/base/content/test/urlbar/browser_page_action_menu.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -

RESOLVED FIXED in Firefox 57

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: intermittent-bug-filer, Assigned: adw)

Tracking

({intermittent-failure})

unspecified
Firefox 57
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

1 failures in 1008 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 1

Platform breakdown:
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-07-24&endday=2017-07-30&tree=all
See Also: → bug 1385882
1 failures in 888 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 1

Platform breakdown:
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-07-31&endday=2017-08-06&tree=all
2 failures in 949 pushes (0.002 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 2

Platform breakdown:
* linux64: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-08-14&endday=2017-08-20&tree=all
2 failures in 908 pushes (0.002 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2

Platform breakdown:
* linux64-stylo: 1
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-08-21&endday=2017-08-27&tree=all
(Assignee)

Updated

a year ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
8 failures in 939 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 5
* try: 2
* mozilla-central: 1

Platform breakdown:
* linux32-stylo: 4
* windows8-64: 1
* linux64-stylo: 1
* linux64: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-08-28&endday=2017-09-03&tree=all
8 failures in 924 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 5
* mozilla-inbound: 3

Platform breakdown:
* linux64: 5
* windows7-32: 1
* linux64-stylo-disabled: 1
* linux32-stylo: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-09-04&endday=2017-09-10&tree=all
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
My first thought was to split this up, but I'd like to try removing the setTimeout(5000) that we use when showing subviews and instead wait until the bounds of all the subviews' children become non-zero -- until all the children become visible.  IIRC I tried doing that in another test though and it wasn't sufficient, so this might not work.  It seems to work locally though.  I'll push to try and see.

And actually this test doesn't seem to be opening a lot of subviews, so it's possible that this is timeout is happening because 5000ms is actually too short in some rare cases.  Seems hard to believe though.
(Assignee)

Comment 10

a year ago
Try looks OK so far...  If we land this and failures go up, we can always back it out.
(Assignee)

Updated

a year ago
Attachment #8908419 - Flags: review?(gijskruitbosch+bugs)

Comment 11

a year ago
mozreview-review
Comment on attachment 8908419 [details]
Bug 1384464 - Intermittent browser/base/content/test/urlbar/browser_page_action_menu.js.

https://reviewboard.mozilla.org/r/180050/#review185418

::: browser/base/content/test/urlbar/head.js:280
(Diff revision 1)
>    return new Promise(resolve => {
> -    BrowserPageActions.panelNode.addEventListener("ViewShown", (event) => {
> -      let target = event.originalTarget;
> -      window.setTimeout(() => {
> -        resolve(target);
> +    info("promisePageActionViewShown waiting for ViewShown");
> +    BrowserPageActions.panelNode.addEventListener("ViewShown", event => {
> +      let panelViewNode = event.originalTarget;
> +      resolve(panelViewNode);
> -      }, 5000);
>      }, { once: true });

you can use:

```js
BrowserTestUtils.waitForEvent(BrowserPageActions.panelNode, "ViewShown")
```

which will do this for you. It'll resolve with the `event`, so you can use its `originalTarget` as you do here to figure out the panel view.

::: browser/base/content/test/urlbar/head.js:292
(Diff revision 1)
> +    info("promisePageActionViewShown waiting for all child nodes to be visible");
> +    await BrowserTestUtils.waitForCondition(() => {
> +      let bodyNode = panelViewNode.firstChild;
> +      for (let childNode of bodyNode.childNodes) {
> +        if (!childNode.hidden &&
> +            getComputedStyle(childNode).display != "none") {

`getComputedStyle` will cause a style flush. Does it work without that? I'd expect things that get that styling to have 0 bounds anyway, so the getBoundsWithoutFlushing call (and checks for those bounds to be >0) should sort them out, right?
Attachment #8908419 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 12

a year ago
Well, the send to device subview has items that are specifically set to `display: none` in certain cases here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#1399

So what my patch is doing is ignoring those items (along with hidden=true items for good measure, but we don't currently have any such items I think), and then waiting for the items that should be shown to become visible.  Without the `display: none` check, the test waits for items to become visible that should not become visible.

Is it important to avoid a style flush here?  Can you think of another way to do this?  If not, I'll add a code comment explaining what's going on.
(Assignee)

Updated

a year ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 13

a year ago
(In reply to Drew Willcoxon :adw from comment #12)
> Well, the send to device subview has items that are specifically set to
> `display: none` in certain cases here:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> css#1399
> 
> So what my patch is doing is ignoring those items (along with hidden=true
> items for good measure, but we don't currently have any such items I think),
> and then waiting for the items that should be shown to become visible. 
> Without the `display: none` check, the test waits for items to become
> visible that should not become visible.

Ah, sorry, I misread the code, this explanation makes sense.

> Is it important to avoid a style flush here?  Can you think of another way
> to do this?  If not, I'll add a code comment explaining what's going on.

Well, do we need to wait for all the non-hidden non-display: none items to be visible? Or does waiting for 1 such item suffice? I can't quite work out if you're trying to wait for:

1) the subview to be really, positively, shown
2) the send to device subview to be populated with devices to send to (assuming that's happening after ViewShown)

If it's the first one, I guess we could wait for just 1 non-0 bound item, maybe?

If it's the second one, then we should keep the flush for now, yeah, though a comment would help make sense of the code. :-)

Really, just like we have a way of inspecting the layout tree without flushing (ie getBoundsWithoutFlushing) we should have a way of getting the style information for a node without flushing, but I don't know that we do, and as this is a test-only usage I don't think it matters much here.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(adw)

Updated

a year ago
Blocks: 1380021
3 failures in 1032 pushes (0.003 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 2
* try: 1

Platform breakdown:
* linux32: 2
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-09-11&endday=2017-09-17&tree=all
(Assignee)

Comment 15

a year ago
(In reply to :Gijs from comment #13)
> 1) the subview to be really, positively, shown

This one, yes, so that synthesized clicks on items in the subview actually click the items they're intended to click.  Otherwise we time out.

> If it's the first one, I guess we could wait for just 1 non-0 bound item,
> maybe?

That should be fine, yeah.  I'll modify the patch to wait for the first non-zero-bounds item.  (I still think it's not at all important to avoid flushing in tests in general, and in this case. :-))
Flags: needinfo?(adw)

Comment 16

a year ago
(In reply to Drew Willcoxon :adw from comment #15)
> > If it's the first one, I guess we could wait for just 1 non-0 bound item,
> > maybe?
> 
> That should be fine, yeah.  I'll modify the patch to wait for the first
> non-zero-bounds item.  (I still think it's not at all important to avoid
> flushing in tests in general, and in this case. :-))

Yeah, I'm torn on this one. I've been burned a few times too often by webextension tests doing "strange" things to panels (like closing them synchronously from ViewShowing handlers) causing strange intermittents/errors when making what seems like an innocent change to panelmultiviews, so I'm trying to avoid adding more of those cases. The less we 'interfere' with the panel from the test code (e.g. by forcing layout flushes), the better, if it's not overly complex. I think looking for 1 non-0 item here will be sufficient and hopefully slightly simpler code-wise. :-)

Comment 17

a year ago
(the flip side of this is that if that I don't want that to hold you up unnecessarily, so if it's a giant pain then just land with the sync flushes)
(Assignee)

Comment 18

a year ago
True!  If the flushes modify the behavior you're testing, that's no good, good point.
Comment hidden (mozreview-request)
https://hg.mozilla.org/mozilla-central/rev/a952a23d3f14
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
1 failures in 943 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 1

Platform breakdown:
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1384464&startday=2017-09-18&endday=2017-09-24&tree=all
You need to log in before you can comment on or make changes to this bug.