1368650 - Sometimes can not open Dev tools in a tab (F12 and Inspect Element doesn't work)

Status: NEW

(DevTools :: Inspector, defect, P2)

Description

[Mehdi]

Sometimes I can not open Dev tools using F12/Inspect element. I have to open same url in a new tab for working with Dev tools. I think it began from v 52.x, but I'm not sure.

Comment 1

[Brian Grinstead [:bgrins]]

Is there a particular URL you see this on, or a set of steps that cause the toolbox not to open?  Also, do you see any error message in the Browser Console?

Comment 2

[Mehdi]

@Brian, as I know it's impossible to re-create the bug, so you need to wait until I face the issue again, then I'll post the Browser Console log here.

And no, there is no particular URL, and I don't know, but I don't remember I did set of steps, but I'll answer this later.

Comment 3

[Patrick Brosset <:pbro>]

DevTools bug triage (filter on CLIMBING SHOES).

Comment 4

[Mehdi]

Attachment: bugzilla_1368650.txt

Browser console log, when I faced the issue.

Comment 5

[Mehdi]

Notice that on the tab, that bug happens on it, event after refresh the page, Dev tool doesn't open (via context menu or F12)

Comment 6

[Brian Grinstead [:bgrins]]

Thanks for the log!  Looks like this is inspector related so going to move it over there and see if we can sort anything out from the stack. And please do follow up if you find steps to reproduce the problem.

Comment 7

[Mehdi]

@Brian please take a look @ bug #1376744

Comment 8

[Mehdi]

I just want to say I faced this issue multiple times in v55 too.

Comment 9

[Mehdi]

I faced this issue every day (more than 1 time) and its boring!

So why this issue is still UNCONFIRMED?

I'm using v56.x, btw.

Comment 10

[Patrick Brosset <:pbro>]

Sorry, you are right this is not UNCONFIRMED, but so far no one has been able to replicate this issue, so it's not an easy thing to investigate.
I've had a look at the logs, but couldn't deduce anything that would lead to a fix.

Maybe answering these questions could help a bit:
- does that happen only in tabs where devtools had been opened once before, and then closed?
- do you have e10s turned on? (I think you can check for the value of the browser.tabs.remote.autostart pref in about:config to know this)

Comment 11

[Brian Grinstead [:bgrins]]

By the way, I triggered the same problem in Dev Edition 57 when working on a jekyll site locally (over a ruby development webserver via `jekyll run serve`).  I was basically making some CSS changes in the inspector, saving them back to the CSS file, reloading the page, etc. It's hard to reproduce this specifically, but maybe these logs are enough to make the shutdown not throw.

error occurred while processing 'detach: TypeError: this.walker is null
Stack: get conn@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector.js:214:5
parent@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:775:5
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:860:18
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:949:5
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector.js:223:5
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:877:9
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:949:5
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/styles.js:88:5
destroy@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:877:9
destroy@resource:/  main.js:1655

But even better may be to make errors during actor destroy not completely break future toolboxes on that tab. This can be simulated by manually throwing an error in PageStyleActor.destroy (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.js#84). Julian or Alex, any ideas on how/if we could accomplish that?

Comment 12

[Julian Descottes [:jdescottes]]

Your scenario seems different from the Bug's summary. The summary seems to be about DevTools not opening (so I guess they were not previously opened on this tab). comment 11 seems to be about devtools breaking after reloading the page.

Brian: can you give a bit more details about the state in which the toolbox was after reloading? blank inspector? not responding to shortcuts to hide it?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #12)
> Your scenario seems different from the Bug's summary. The summary seems to
> be about DevTools not opening (so I guess they were not previously opened on
> this tab). comment 11 seems to be about devtools breaking after reloading
> the page.
> 
> Brian: can you give a bit more details about the state in which the toolbox
> was after reloading? blank inspector? not responding to shortcuts to hide it?

To clarify, I do this a few times:

- Make some CSS changes in the inspector
- Edit the CSS file on disk
- Reload the page

Then at some stage I close the toolbox, triggering the error in Comment 11.  After that, I can no longer open the toolbox on that tab (keyboard shortcut or otherwise), I need to open a new tab and paste in the URL in order to get a toolbox again.  I don't think (but am not positive) there's any error log showing up when trying to re-open the toolbox, I think the target never fully gets shut down when I closed the toolbox and it's preventing it from re-opening.

Comment 14

[Alexandre Poirot [:ochameau]]

I suspect DOM Promises from the panels to be a source of issues.
When closing the toolbox, they end up being frozen, whereas the Promises from privileged sandbox are always alive.
While working on getting rid of Promise.jsm, I introduced even more usaged of document promises instead of Promise.jsm ("frozable" versus "always-alive"). The result was that the toolbox more easily freeze/break at shutdown.

Unfortunately, fixing that is really hard. Toolbox and panel destruction codepaths are very complex. And this is even more unfortunate because this complexity if mostly because of tests that fails in many ways anytime you touch this codepath.
In production we could possibly just do nothing at shutdown and be fine.

Having said that, in the meantime we may just fix all the stack traces we see blindly.
Here it may just be that we try to destroy the NodeActor more than once,
would it fix your breakage if we do this from NodeActor.destroy:

  destroy: function () {
    // NodeActor may already be destroyed
    if (!this.walker) {
      return;
    }
    protocol.Actor.prototype.destroy.call(this);

?

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> I suspect DOM Promises from the panels to be a source of issues.
> When closing the toolbox, they end up being frozen, whereas the Promises
> from privileged sandbox are always alive.
> While working on getting rid of Promise.jsm, I introduced even more usaged
> of document promises instead of Promise.jsm ("frozable" versus
> "always-alive"). The result was that the toolbox more easily freeze/break at
> shutdown.

Mehdi did mention seeing this in 55 - were we using DOM Promises then as well?

> Unfortunately, fixing that is really hard. Toolbox and panel destruction
> codepaths are very complex. And this is even more unfortunate because this
> complexity if mostly because of tests that fails in many ways anytime you
> touch this codepath.
> In production we could possibly just do nothing at shutdown and be fine.
> 
> Having said that, in the meantime we may just fix all the stack traces we
> see blindly.
> Here it may just be that we try to destroy the NodeActor more than once,
> would it fix your breakage if we do this from NodeActor.destroy:
> 
>   destroy: function () {
>     // NodeActor may already be destroyed
>     if (!this.walker) {
>       return;
>     }
>     protocol.Actor.prototype.destroy.call(this);

I don't know if it would fix it - it's hard to test since there aren't reliable STR. That said, if we know that actors can be destroyed more than once it seems reasonable to land this change in Nightly and try to get confirmation from Mehdi. 

Could we make a change to the protocol to guard against actors being detroyed twice? I suspect that we have many actors that don't account for this, and I'm not sure there's a reason why they should have to.

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Mehdi did mention seeing this in 55 - were we using DOM Promises then as
> well?

I think panels eagerly used them. But gien that Mehdi has a stack with an exception,
this is most likely because of an exception and not a frozen promise.

> I don't know if it would fix it - it's hard to test since there aren't
> reliable STR. That said, if we know that actors can be destroyed more than
> once it seems reasonable to land this change in Nightly and try to get
> confirmation from Mehdi.

+1

> Could we make a change to the protocol to guard against actors being
> detroyed twice? I suspect that we have many actors that don't account for
> this, and I'm not sure there's a reason why they should have to.

protocol.js already has some guards:
http://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#864

But here, I think this is very specific to inspector.
It may come from this destroy call:
http://searchfox.org/mozilla-central/source/devtools/server/actors/inspector.js#1331
I think we do our best to call releaseNode only once per NodeActor, but it is hard to be sure.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1368650 - Prevent frozen request when actor throws.

When an exception happen, the server replies with {"error":"unknownError"} message,
but without any actorID. It is considered as an unsolicited message.
The original client request that ends up throwing on the server ends up never resolving.
By passing back "actor" with the original actorID being requested, we allow the client to resolve with an error.

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

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1368650 - Prevent NodeActor.destroy from throwing when called more than once.

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

Comment 19

[Alexandre Poirot [:ochameau]]

I would like to add tests for the first patch (fix client requests throwing on the actors) before moving forward.
This patch should fix a bunch of similar issues.


And I don't know about the second (fix NodeActor.destroy when called multiple times).
I'm not sure it is really this. I'm not sure it is because NodeActor.destroy is called multiple times.
The stack mention the usage of "parent" and "conn" from protocol.js. Is NodeActor parent of any other actor? I thought it was only the child of Inspector actor??

Comment 20

[Julian Descottes [:jdescottes]]

I just had the issue here while I was using the debugger.

I popped up the browser toolbox and debugged a bit. The reason devtools don't open is that the old toolbox is still registered.
Probably because we never got 

  toolbox.once("destroyed", () => {
    this._toolboxes.delete(target);
    this.emit("toolbox-destroyed", target);
  });

(sorry if that was obvious for everybody else :) )

As Alex said, the destroy code of the toolbox is pretty complex. Some things we might want to do here:
- emit destroyed in the catch at http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/devtools/client/framework/toolbox.js#2734
- switch the toolbox destroy to an async function and try catch around it (and emit "destroyed" in the catch too)
- if we are afraid that some nested async destroys can silently fail we could also emit destroyed after a given timeout?

Now the question is whether it's safe to remove the toolbox from the gDevtools _toolboxes map even if the destroy failed.

If emitting "destroyed" is too optimistic and reopening the toolbox after a bad destroy is risky. In this case we could send another event in all the cases I mentioned above and display a message to our users ("DevTools failed to close properly on this tab, please use a new tab etc...")

For the record my stack trace here was:

>  onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'evaluateJS: Error: Debugger.Frame is not live\nStack: evalWithDebugger@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:1357:11\nonEvaluateJS@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:917:20\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1791:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:761:7\nLine: 1357, column: 11"}
>  Stack: onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:787:9
>  send/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:570:13
>  exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
>  exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #20)
> I just had the issue here while I was using the debugger.
> 
> I popped up the browser toolbox and debugged a bit. The reason devtools
> don't open is that the old toolbox is still registered.
> Probably because we never got 
> 
>   toolbox.once("destroyed", () => {
>     this._toolboxes.delete(target);
>     this.emit("toolbox-destroyed", target);
>   });
> 
> (sorry if that was obvious for everybody else :) )
> 
> As Alex said, the destroy code of the toolbox is pretty complex. Some things
> we might want to do here:
> - emit destroyed in the catch at
> http://searchfox.org/mozilla-central/rev/
> ed212c79cfe86357e9a5740082b9364e7f6e526f/devtools/client/framework/toolbox.
> js#2734
> - switch the toolbox destroy to an async function and try catch around it
> (and emit "destroyed" in the catch too)
> - if we are afraid that some nested async destroys can silently fail we
> could also emit destroyed after a given timeout?
> 
> Now the question is whether it's safe to remove the toolbox from the
> gDevtools _toolboxes map even if the destroy failed.
> 
> If emitting "destroyed" is too optimistic and reopening the toolbox after a
> bad destroy is risky. In this case we could send another event in all the
> cases I mentioned above and display a message to our users ("DevTools failed
> to close properly on this tab, please use a new tab etc...")

Yes, I'd rather fix this globally if possible. Of course we should track down and fix individual shutdown exceptions as we find them but the result is bad enough that IMO we should be error tolerant from the framework level / client side.

To test this scenario and see what happens if we unconditionally emit "destroyed", we can add `throw "Shutdown error"` in an actor destroy() function i.e.: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.js#84.

Comment 22

[Josh Keaton]

Attachment: blotchacadrutp19897230.txt

Thanks from unagevni1988.