Closed Bug 1160345 Opened 5 years ago Closed 4 years ago

Inspect element / command buttons no longer shown for old servers

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jryans, Assigned: bgrins)

References

Details

(Keywords: regression, Whiteboard: [polish-backlog])

Attachments

(1 file, 2 obsolete files)

It appears that bug 1128988 (GCLI for e10s) does not properly support backwards compatibility for old servers.

For example, if I start Simulator 2.2 from WebIDE and connect to an app, Inspect Element is missing and the server gives the error:

"Protocol error (unrecognizedPacketType): Actor conn0.child21/gcliActor11 does not recognize the packet type specs" protocol.js:20
onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"Error occurred while creating actor 'undefined: TypeError: browser is undefined\nStack: exports.GcliActor<.initialize@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/gcli.js:1:952\nconstructor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js:16:80\nObservedActorFactory.prototype.createActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/common.js:13:99\nDebuggerServerConnection.prototype._getOrCreateActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:81:53\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:91:11\nChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:38:288\nLine: 1, column: 951"}
Stack: DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:944:1
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:471:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
Line: 944, column: 0

We need to restore previous functionality in some way here.
Other command buttons missing include:

* Window / frame switching
* Split console
Summary: Inspect element no longer shown for old servers → Inspect element / command buttons no longer shown for old servers
See Also: → 1157906
I am not sure what the correct way to proceed here is.

How should we restore the command buttons for old servers?
Flags: needinfo?(jwalker)
Flags: needinfo?(bgrinstead)
Prior to the addition of runAt support, we did this:

https://hg.mozilla.org/mozilla-central/diff/6ecd33215340/browser/devtools/shared/DeveloperToolbar.jsm#l1.84

I think that logic really belongs in toolbox.js, probably somewhere like this:

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#716

> How should we restore the command buttons for old servers?

Are we sure they were there before? If they were, then it must have been via a mechanism other than GCLI.
Flags: needinfo?(jwalker)
I'll fix this, it's something really simple
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
Attached patch toolbox-buttons.patch (obsolete) — Splinter Review
The problem is just that the call to gcli is blocking the call to setToolboxButtonsVisibility (which sets the non-gcli buttons like inspect 
element).

Here I just make that call and then bail out of the function if it's going to fail (these buttons would have never worked previously anyway since gcli wasn't remote safe).

Ryan, is there a better way to detect the presence of the actor than using a trait as I have here?
Attachment #8600351 - Flags: feedback?(jryans)
See Also: → 1160361
Attached patch toolbox-buttons.patch (obsolete) — Splinter Review
Using target.hasActor instead of a trait
Attachment #8600351 - Attachment is obsolete: true
Attachment #8600351 - Flags: feedback?(jryans)
Attachment #8600372 - Flags: review?(jryans)
Comment on attachment 8600372 [details] [diff] [review]
toolbox-buttons.patch

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

This doesn't seem quite right.  The trouble here is that there *was* in fact a GCLI actor[1] before bug 1128988.  After 1128988, the actor was heavily changed, and so far it's unclear to me if we still know how to connect to the old version of it at all.

Or maybe you mean that we *should* be able to connect to the old actor, but there are just more things to fix before it actually works?  With this patch applied and starting simulator 2.2 and opening an app, I see the same "browser is undefined" error from comment 0.  Are we even meant to use the old GCLI actor (pre bug 1128988) remotely like this?  If not, then the trait version seems more appropriate.

This patch does restore:

* Inspect element
* Frame switching

However:

* Split console

is still missing.

If you try Dev. Edition (39) with Simulator 2.2, you can see the split console button was there.  When I try Nightly w/ this patch applied, it's not there anymore.

[1]: https://hg.mozilla.org/mozilla-central/filelog/7723b15ea695/toolkit/devtools/server/actors/gcli.js
Attachment #8600372 - Flags: review?(jryans)
Blocks: 1160559
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Or maybe you mean that we *should* be able to connect to the old actor, but
> there are just more things to fix before it actually works?  With this patch
> applied and starting simulator 2.2 and opening an app, I see the same
> "browser is undefined" error from comment 0.  Are we even meant to use the
> old GCLI actor (pre bug 1128988) remotely like this?  If not, then the trait
> version seems more appropriate.

Or, if the new GCLI actor has a new method that did not exist before, you could possibly use target.actorHasMethod to check for it.  Though, this might be less clear than a trait if you are just picking a random method you don't actually need at the moment as a stand-in for "is new".
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8600372 [details] [diff] [review]
> toolbox-buttons.patch
> 
> Review of attachment 8600372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem quite right.  The trouble here is that there *was* in fact
> a GCLI actor[1] before bug 1128988.  After 1128988, the actor was heavily
> changed, and so far it's unclear to me if we still know how to connect to
> the old version of it at all.

Hm, good catch.  The intention was to have the same behavior as the trait, but that isn't working as you point out.  I'm not sure exactly what the solution is for this yet, but maybe we should first land a patch that just toggles the visibility correctly and then do the split console (aka old gcli actor support) separately
(In reply to Brian Grinstead [:bgrins] from comment #9)
> but maybe we should first land a patch that just
> toggles the visibility correctly and then do the split console (aka old gcli
> actor support) separately

The reasoning being that the 'inspect element' button missing is a major problem for anyone using WebIDE
My idea from comment 9 so we can get these buttons back in the next nightly.  I will look further into getting the split console button to show up in a separate patch.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb7e552ead12
Attachment #8600372 - Attachment is obsolete: true
Attachment #8600394 - Flags: review?(jryans)
If the only remaining button that doesn't show up with an old server is the split console, one option would be to proceed by decoupling that button from gcli altogether.
Comment on attachment 8600394 [details] [diff] [review]
toolbox-buttons.patch

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

Great, this makes sense to me.

It also brings the buttons back for new b2g servers (bug 1160361).

Should I file a separate bug for split console / other command buttons?
Attachment #8600394 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> It also brings the buttons back for new b2g servers (bug 1160361).

I updated the title of bug 1160361, since the patch here will also restore inspect element for new b2g servers.  Bug 1160361 is now just about getting the GCLI actor to work right on b2g.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> > It also brings the buttons back for new b2g servers (bug 1160361).
> 
> I updated the title of bug 1160361, since the patch here will also restore
> inspect element for new b2g servers.  Bug 1160361 is now just about getting
> the GCLI actor to work right on b2g.

OK, sounds great
https://hg.mozilla.org/mozilla-central/rev/f9996ba3e1c0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
See Also: → 1161131
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> Should I file a separate bug for split console / other command buttons?

Since this bug is now marked fixed, I filed bug 1161131 for the remaining "command buttons / split console" issue.
Whiteboard: [devedition-40] → [polish-backlog]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.