1284259 - EventDetailsTooltip intermittent error in EventTooltip.destroy

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1267403. Below, the relevant discussion.

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Comment on attachment 8764562 [details]
> Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL
> document;
> 
> https://reviewboard.mozilla.org/r/60360/#review58422
> 
> Tested with color,cubic,filter tooltips works great.
> Also looked at event details and images. Is there any others?
> 
> Note that while testing event details I've seen various weird exception.
> But I don't think it is related to this patch:
>   TypeError: can't convert undefined to object:
> EventTooltip.prototype.destroy@resource://gre/modules/commonjs/toolkit/
> loader.js ->
> resource://devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:293:
> 24
> It happens when clicking outside the tooltip to dismiss it. It is
> intermittent and doesn't always throws...
> 

Indeed thanks for the heads up. There is a race condition which can lead to erase the tooltip content before having destroyed the previous tooltip. Comes from two reasons: 
- calling "hide" on a Tooltip not yet visible is not triggering the "hidden" event (early return in the hide() method)
- in markup.js, on event bubble click handler, we hide() the tooltip, then fetch the event info asynchronously and then show() the tooltip. We should hide() right before calling show() otherwise we are exposed to race conditions

Comment 1

[Marco Mucci [:MarcoM]]

Waiting for team triage.

Comment 2

[Alexandre Poirot [:ochameau]]

Also taking this easy one.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Ooops, I forgot to attach the patch and r? it...

So it looks like waiting for tooltip.hide() before proceeding is enough to fix that.
I also removed the unused returned value.

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8770461 [details] [diff] [review]
patch v1

Review of attachment 8770461 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I can still reproduce the issue when double clicking quickly on an event bubble.
I think we should keep your patch, since it already improves the situation, but I would like to submit an additional patch to fix the remaining issue I describe below.

If you manage to click quickly enough so that _buildEventTooltipContent is called twice:
- both calls try to hide the tooltip (but no tooltip displayed at this point)
- both perform the async call to getEventListenerInfo
- both create an EventTooltip and call tooltip.show

Now next time you click out of the tooltip, the tooltip will fire it's "hidden" event and both EventTooltipInstances will be destroyed. One of them will fail because this._tooltip.eventEditors has been overridden by the other tooltip. See the EventTooltip constructor:

> this._tooltip.eventEditors = new WeakMap();

To completely fix the issue the eventEditors map should not be stored on this._tooltip, but directly on the EventTooltip instance.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1284259 - store eventEditors map on EventTooltip instance to fix destroy failures;

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

Comment 6

[Julian Descottes [:jdescottes]]

Pushed to try both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b2614d03ff

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8770581 [details]
Bug 1284259 - store eventEditors map on EventTooltip instance to fix destroy failures;

https://reviewboard.mozilla.org/r/64026/#review60996

Please also modify devtools/client/inspector/markup/test/helper_events_test_runner.js before landing.
But I'm afraid you may not have access to EventTooltip instance from there?

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8770581 [details]
Bug 1284259 - store eventEditors map on EventTooltip instance to fix destroy failures;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64026/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/64026/#review60996

Right good catch! So I propose to add a reference to the EventTooltip on the HTMLTooltip instance, and document it as being used in tests. 

I don't want to make the EventTooltip instance visible from the MarkupContainer/MarkupView. Let me know if you're ok with this.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8770581 [details]
Bug 1284259 - store eventEditors map on EventTooltip instance to fix destroy failures;

https://reviewboard.mozilla.org/r/64026/#review61006

I let you land both patches at once.

Comment 11

[Julian Descottes [:jdescottes]]

Thanks for the review! Will land both patches after a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0212d3656d1f

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8907ac7cdaee
Prevent exception while dismissing event tooltip details. r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/3e1f197a8eba
store eventEditors map on EventTooltip instance to fix destroy failures;r=ochameau

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8907ac7cdaee
https://hg.mozilla.org/mozilla-central/rev/3e1f197a8eba