Closed Bug 1384464 Opened 7 years ago Closed 7 years ago

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. -

Categories

(Firefox :: Address Bar, defect, P5)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

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

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1384464 - Intermittent browser/base/content/test/urlbar/browser_page_action_menu.js.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
See Also: → 1385882
Assignee: nobody → adw
Status: NEW → ASSIGNED
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.
Try looks OK so far... If we land this and failures go up, we can always back it out.
Attachment #8908419 - Flags: review?(gijskruitbosch+bugs)
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+
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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)
Blocks: 1380021
(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)
(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. :-)
(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)
True! If the flushes modify the behavior you're testing, that's no good, good point.
https://hg.mozilla.org/mozilla-central/rev/a952a23d3f14
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: