Closed Bug 1284259 Opened 4 years ago Closed 4 years ago

EventDetailsTooltip intermittent error in EventTooltip.destroy

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- fixed

People

(Reporter: jdescottes, Assigned: ochameau)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files)

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
Flags: qe-verify-
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
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Attached patch patch v1Splinter Review
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+
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?
Attachment #8770581 - Flags: review?(poirot.alex)
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/
Attachment #8770581 - Flags: review?(poirot.alex)
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 jdescottes@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8907ac7cdaee
https://hg.mozilla.org/mozilla-central/rev/3e1f197a8eba
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.