Closed Bug 1284259 Opened 5 years ago Closed 5 years ago
Details Tooltip intermittent error in Event Tooltip .destroy
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
Whiteboard: [devtools-html] [triage]
Component: Developer Tools: Framework → Developer Tools: Shared Components
Priority: -- → P2
Waiting for team triage.
Severity: normal → enhancement
Priority: P2 → --
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Also taking this easy one.
Assignee: nobody → poirot.alex
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.
Attachment #8770461 - Flags: review?(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.
Attachment #8770461 - Flags: review?(jdescottes) → review+
Review commit: https://reviewboard.mozilla.org/r/64026/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64026/
Pushed to try both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b2614d03ff
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 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/
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 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.
Attachment #8770581 - Flags: review?(poirot.alex) → review+
Thanks for the review! Will land both patches after a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0212d3656d1f
Pushed by firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.