Closed Bug 1381020 Opened 2 years ago Closed 2 years ago

Permanent failure on Cedar: browser/components/uitour/test/browser_UITour2.js | Timeout waiting for popup at anchor: Info panel should be anchored to the customize button

Categories

(Firefox :: Tours, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(1 file, 1 obsolete file)

Strangely, this seems to only affect Linux64.

Snipped from log:

[task 2017-07-14T00:36:38.688694Z] 00:36:38     INFO - TEST-START | browser/components/uitour/test/browser_UITour2.js
[task 2017-07-14T00:36:42.743064Z] 00:36:42     INFO - TEST-INFO | started process screentopng
[task 2017-07-14T00:36:43.314472Z] 00:36:43     INFO - TEST-INFO | screentopng: exit 0
[task 2017-07-14T00:36:43.316442Z] 00:36:43     INFO - Buffered messages logged at 00:36:38
[task 2017-07-14T00:36:43.317683Z] 00:36:43     INFO - Starting test_info_customize_auto_open_close
[task 2017-07-14T00:36:43.318113Z] 00:36:43     INFO - Buffered messages finished
[task 2017-07-14T00:36:43.320085Z] 00:36:43     INFO - TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour2.js | Timeout waiting for popup at anchor: Info panel should be anchored to the customize button - 
[task 2017-07-14T00:36:43.322065Z] 00:36:43     INFO - Stack trace:
[task 2017-07-14T00:36:43.327118Z] 00:36:43     INFO - chrome://mochitests/content/browser/browser/components/uitour/test/head.js:waitForCondition/<:39
[task 2017-07-14T00:37:23.705798Z] 00:37:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-07-14T00:37:23.708946Z] 00:37:23     INFO - TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour2.js | Test timed out - 
[task 2017-07-14T00:37:23.849107Z] 00:37:23     INFO - GECKO(2482) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2017-07-14T00:37:23.852220Z] 00:37:23     INFO - GECKO(2482) | MEMORY STAT | vsize 2107MB | residentFast 232MB | heapAllocated 97MB
[task 2017-07-14T00:37:23.853653Z] 00:37:23     INFO - TEST-OK | browser/com

Example job: https://treeherder.mozilla.org/#/jobs?repo=cedar&selectedJob=114031779
Hey MattN, this is apparently the range that first introduced this perma-orange on cedar:

https://hg.mozilla.org/projects/cedar/pushloghtml?changeset=1b8b48d86d9d6cd0acb286ace1fdd531bdcd8c12

A regressor isn't jumping out at me except perhaps the onboarding stuff... do you know who I should be pointing this bug at?
Flags: needinfo?(MattN+bmo)
This seems to be caused by this backout:

https://hg.mozilla.org/mozilla-central/rev/632562f2d03b

Which happened in bug 1377802.

So I _suspect_ we need to modify UITour (or the test) to wait for the anchor to be visible before attempting to open the tooltip?

Does that sound right, johannh?
Blocks: 1377802
Flags: needinfo?(jhofmann)
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #2)
> This seems to be caused by this backout:
> 
> https://hg.mozilla.org/mozilla-central/rev/632562f2d03b
> 
> Which happened in bug 1377802.
> 
> So I _suspect_ we need to modify UITour (or the test) to wait for the anchor
> to be visible before attempting to open the tooltip?
> 
> Does that sound right, johannh?

Yeah, I can only assume that there's some sensitive timing involved where the tooltip is sometimes showing before the anchor.  With the backout, the tooltip will be hidden without an anchor, without the backout, it will be placed at a semi-random position on the screen. It seems like UITour authors already took steps to prevent that: https://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/browser/components/uitour/UITour.jsm#1279

I can't reproduce any of this locally on Linux64, both the actual feature and the test work fine for me. The weirdest part is that this test already existed for a long time before the backouted code was originally written. It should not have any effect on it either way.

Considering this and considering that the backout fixes an important issue with permission doorhangers persisting through navigation, I vote for disabling the test on Cedar, unless someone can come up with a better explanation or can reproduce the problem reliably.
Flags: needinfo?(jhofmann)
Okay, I think I've figured this one out.

Apparently, for the customize mode tooltip, we anchor the tour popup on the toolbarbutton icon in the customize button. If the image for that tooltip hasn't yet loaded, then the icon has a 0-sized rect, and we fail to anchor the popup, which causes the tooltip to close again.

Hey MattN, if I change the UITour to point at the entire button instead of the icon, all is well... since the old customize panel is going away soon anyways, is it alright if we just point at the entire button?
Assignee: nobody → mconley
Comment on attachment 8887129 [details]
Bug 1381020 - Anchor UITour tooltip for customize button to whole button instead of icon to avoid a race.

https://reviewboard.mozilla.org/r/157882/#review163168

::: browser/components/uitour/UITour.jsm:148
(Diff revision 1)
>            return aDocument.getElementById("appMenu-customize-button");
>          }
> -        let customizeButton = aDocument.getElementById("PanelUI-customize");
> +        return aDocument.getElementById("PanelUI-customize");

This is fine with me but since we are using UITour for onboarding in Fx56, I think we should confirm with Verdi if this is okay.
Attachment #8887129 - Flags: review?(MattN+bmo) → review+
Thanks MattN.

Hey verdi, ni? to you on comment 5.
Flags: needinfo?(MattN+bmo) → needinfo?(mverdi)
Talked this over with verdi and thought about it a bit more.

I think the real problem here is that UITour will attempt to open popups on things that might not have primary frames yet, and that's kinda problematic in general - not just for the customize button case.

We probably want a more general solution here. :/

MattN - would you be fine if UITour polls for a rect on the tooltip anchor via requestIdleCallback and getBoundsWithoutFlushing for some amount of tries before attempting to open the tooltip?
Flags: needinfo?(mverdi) → needinfo?(MattN+bmo)
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #9)
> Talked this over with verdi and thought about it a bit more.
> 
> I think the real problem here is that UITour will attempt to open popups on
> things that might not have primary frames yet, and that's kinda problematic
> in general - not just for the customize button case.
> 
> We probably want a more general solution here. :/

Do you understand why this only started failing recently? I looked a bit at bug 1377802 but don't understand the root cause of this bug.

> MattN - would you be fine if UITour polls for a rect on the tooltip anchor
> via requestIdleCallback and getBoundsWithoutFlushing for some amount of
> tries before attempting to open the tooltip?

Do you mean replace the visibility check with polling for visibility? If so, I think that's fine assuming it doesn't require a big refactoring.
Flags: needinfo?(MattN+bmo) → needinfo?(mconley)
Ugh, so, developments.

This has _nothing_ to do with image loading delay. The reason why the <xul:image> is 0x0 is because it's been removed from the DOM. In the case of the customize button icon target, this is because when we open the menu panel, we move the nodes from one part of the DOM to another, which causes the anonymous content we've already got a reference to to go out of date[1]. Hilarious.

[1]: We hold a reference to the anonymous content. The binding is destroyed on the move, and re-applied once it gets its final resting place in the DOM, and now our reference is out of date.
Copying some of my comments from IRC:

[15:22:36] <MattN> mconley: oh, so I can see how this could happen since we `getTarget` before calling `showInfo` to pass the target as an argument but then `showInfo` opens the panel and at that point we probably shuffle the DOM nodes or new bindings get attached do the panel opening and we don't update our target reference
[15:23:43] <MattN> I'm guessing if we update the node reference inside the inner `showInfoPanel` callback then the problem would go away
[15:23:58] <MattN> (update by calling `getTarget`)
[15:24:36] <MattN> (that isn't a proper solution probably but I think it would work)
[15:25:11] <MattN> i.e. I can see how the target's node reference would be stale if we get it before opening the panel that that target is in
Attachment #8887129 - Attachment is obsolete: true
Comment on attachment 8888065 [details]
Bug 1381020 - Account for UITour panel targets having been invalidated due to DOM shuffle and XBL teardown.

https://reviewboard.mozilla.org/r/158964/#review164382

I have one question but the rest looks fine. There is some risk this will introduce other bugs but since we don't have any tours already in release I'm not worried about regressing those.

::: browser/components/uitour/UITour.jsm:1297
(Diff revision 1)
> -                                       showInfoPanel.bind(this, this._correctAnchor(aAnchor.node)));
> +                                       aAnchor,
> +                                       () => {
> +                                         showInfoPanel.apply(this, [this._correctAnchor(aAnchor.node)]);

Is the showInfoPanel.bind change necessary? It introduces an unnamed function in the stack and IMO the indentation isn't as nice.

If it is necessary then why not .call to avoid the array?
Attachment #8888065 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8888065 [details]
Bug 1381020 - Account for UITour panel targets having been invalidated due to DOM shuffle and XBL teardown.

https://reviewboard.mozilla.org/r/158964/#review164382

> Is the showInfoPanel.bind change necessary? It introduces an unnamed function in the stack and IMO the indentation isn't as nice.
> 
> If it is necessary then why not .call to avoid the array?

Yes, we need this. If we don't, then the aAnchor.node is bound as the `showInfoPanel` function argument before `_setAppMenuStateForAnnotation` has had a chance to refresh it.

I'll use .call and avoid the Array before landing. Thanks!
Comment on attachment 8888065 [details]
Bug 1381020 - Account for UITour panel targets having been invalidated due to DOM shuffle and XBL teardown.

https://reviewboard.mozilla.org/r/158964/#review164382

I've split out the function and given it a comment to explain why we're wrapping yet another function so strangely.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1134bcd9eb88
Account for UITour panel targets having been invalidated due to DOM shuffle and XBL teardown. r=MattN
https://hg.mozilla.org/mozilla-central/rev/1134bcd9eb88
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.