Inspector actors should clean up by default

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: jryans, Assigned: ochameau)

Tracking

unspecified
Firefox 42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Currently, the inspector / highlighter actors require several messages to be exchanged to shut down properly.  We should change this so that the actors shut down correctly without requiring client messages.

As a user visible symptom of this today:

1. Use the inspector to highlight element on an FxOS device
2. Unplug device with highlighter visible
3. The highlighter remains on screen

The server is aware of when the connection dies, so we should ensure these actors shut down and remove things like highlighters when that happens.

Today, we appear to send the message "hideBoxModel" to achieve this, and it ends up left on screen if that message is not received.
Alex, maybe you are doing this in your recent work?
Flags: needinfo?(poirot.alex)
Oh! Yes I'll at least do some of these cleanups. I already have a patch for the hideboxmodel.
Flags: needinfo?(poirot.alex)
Depends on: 1161072
Assignee: nobody → poirot.alex
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #8637997 - Flags: review?(jryans)
Attachment #8637997 - Flags: review?(jryans) → review?(bgrinstead)
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Created attachment 8637997 [details] [diff] [review]
> patch v1

Sorry, I'm not going to be able to review this until next week
Comment on attachment 8637997 [details] [diff] [review]
patch v1

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

Seems fine to me
Attachment #8637997 - Flags: review?(bgrinstead) → review+
Comment on attachment 8637997 [details] [diff] [review]
patch v1

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

::: browser/devtools/markupview/markup-view.js
@@ +1469,5 @@
>      // Note that if the toolbox is closed, this will work fine, but will fail
>      // in case the browser is closed and will trigger a noSuchActor message.
>      // We ignore the promise that |_hideBoxModel| returns, since we should still
>      // proceed with the rest of destruction if it fails.
> +    // FF42+ now does the cleanup from the actor.

Alternatively, could we manage this inside the toolbox's destroyInspector method, which already deals with highlighter destruction: called https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1759?

From there we could call

if (!this.highlighter.traits.autoHideOnDestroy) {
  this.highlighterUtils.unhighlight()
}

In that case we could completely remove the call to _hideBoxModel here, and it may also handle cases where the highlighter was shown from a different tool.  Not sure if it would mess up timings since the tool's destroy is fired before destroyInspector in the toolbox, but it seems like it may be a better place to put it.  Up to you
Posted patch patch v2Splinter Review
It seems to work correctly if I move this code to toolbox.js...
let's see if try is happy with that:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdb20a6ee39a
Attachment #8637997 - Attachment is obsolete: true
Attachment #8639733 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d98c9b5082f1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.