Closed Bug 1240907 Opened 9 years ago Closed 8 years ago

Toolbox should target page content, not RDM tool

Categories

(DevTools :: Responsive Design Mode, defect, P1)

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox50 fixed, firefox51 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(3 files)

Bug 1240907 - Tunnel toolbox messages to content in RDM.
(deleted), text/x-review-board-request
ochameau
: review+
Details
Bug 1240907 - Flatten RemoteBrowserTabActor into BrowserTabActor.
(deleted), text/x-review-board-request
ochameau
: review+
Details
Bug 1240907 - Clean up code style in test-actor-registry.js.
(deleted), text/x-review-board-request
ochameau
: review+
Details 
The basic shell in bug 1239437 does not really interact with the toolbox correctly.

The toolbox should be targeting the page content, not the markup of the RDM tool (which includes a frame with the page).

This should be easier to fix after switching to <iframe mozbrowser> in bug 1240896.
Priority: -- → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Priority: P2 → P1
QA Contact: mihai.boldan
After the page state swap in bug 1240913, the GCLI target is confused.  Fixing this is likely related to fixing the toolbox.

Re-enable browser_resize_cmd.js once it's fixed.
Priority: P1 → P2
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
Iteration: 49.3 - Jun 6 → 50.1
Iteration: 50.1 → 50.2
Actually we also need bug 1240912 for the GCLI test, so we'll re-enable it over there.
Comment on attachment 8767366 [details]
Bug 1240907 - Flatten RemoteBrowserTabActor into BrowserTabActor.

https://reviewboard.mozilla.org/r/61890/#review58786

This patch looks good, do not hesitate to land it before RDM one.

::: devtools/server/actors/webbrowser.js:2195
(Diff revision 1)
> +      let sessionStore = this._browser.__SS_data;
> +      // Get the last selected entry
> +      let entry = sessionStore.entries[sessionStore.index - 1];
> +      return entry.title;
> +    }
> +    let title = this._browser.contentTitle;

Same here. I imagine the title would correctly be retrieved from the child. The only useful thing is handling zombie tabs.

::: devtools/server/actors/webbrowser.js:2222
(Diff revision 1)
> +      let entry = sessionStore.entries[sessionStore.index - 1];
> +      return entry.url;
> +    }
> +    if (this.webNavigation.currentURI) {
> +      return this.webNavigation.currentURI.spec;
> +    }

I imagine this currentURI is a relic from the past? Or just some code specific to in-process?
We should either get a url from the child correctly or from the session store for zombie tabs.

::: devtools/server/actors/webbrowser.js:2233
(Diff revision 1)
> +    if (!form.title) {
> +      form.title = this.title;
> +    }
> +    if (!form.url) {
> +      form.url = this.url;
> +    }

A comment about this title and url would be great.
This is a special case for Firefox, to guess additional metadata from the parent process for unloaded/zombie tabs.
Attachment #8767366 - Flags: review?(poirot.alex) → review+
Comment on attachment 8767365 [details]
Bug 1240907 - Tunnel toolbox messages to content in RDM.

https://reviewboard.mozilla.org/r/61888/#review58880

Network monitor doesn't work.
Also I'm seeing various exceptions when toggle RDM on google:
this.webNavigation is null -- browser.xml:902
JavaScript error: resource://gre/modules/RemoteAddonsParent.jsm, line 1086: TypeError: cannot use the given object as a weak map key
JavaScript error: resource:///modules/ReaderParent.jsm, line 82: TypeError: win.gBrowser is undefined

Some may be related to some of my patches in my queue, but I imagine the first one is related to yours?

I also get this exception when toggle RDM off while tools are opened:
JavaScript error: resource://gre/modules/ExtensionContent.jsm, line 958: TypeError: this.globals.get(...) is undefined
(might be related to one of my patches)

But the console also breaks when doing that, with the following exception:
Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: this.inner.frameLoader is null

Also, I'm wondering if your tunneling approach is going to scale for multiple viewports...
Don't you think it would be easier to pass a precise frame/messageManager to devtools codebase instead of doing this tunneling work? And especially easier when there will be more than just one frame?

::: devtools/client/responsive.html/browser/tunnel.js:411
(Diff revision 1)
> +  addMessageListener(name, ...args) {
> +    debug(`Calling addMessageListener for ${name}`);
> +
> +    debug(`Add outer listener for ${name}`);
> +    // Add an outer listener, just like a simple pass through
> +    this.outerParentMM.addMessageListener(name, ...args);

Is this really useful/wanted if `name` matches a prefix?

::: devtools/client/responsive.html/test/browser/head.js:322
(Diff revision 1)
> +  info("Selecting the node for '" + selector + "'");
> +  let nodeFront = yield getNodeFront(selector, inspector);
> +  let updated = inspector.once("inspector-updated");
> +  inspector.selection.setNodeFront(nodeFront, reason);
> +  yield updated;
> +});

Instead of duplicating code from devtools/client/inspector/test/head.js, could you instead share these helpers?!

::: devtools/server/actors/webbrowser.js:371
(Diff revision 1)
>    }
>  
> -  actor = new BrowserTabActor(this._connection, browser);
> +  actor = new RemoteBrowserTabActor(this._connection, browser);
>    this._actorByBrowser.set(browser, actor);
>    this._checkListening();
> -  return promise.resolve(actor);
> +  return actor.connect();

Shouldn't this be part of the second patch?

::: devtools/server/main.js:985
(Diff revision 1)
>     *         established.
>     */
>    connectToChild(connection, frame, onDestroy) {
>      let deferred = SyncPromise.defer();
>  
> -    let mm = frame.frameLoader.messageManager;
> +    let mm = frame.messageManager || frame.frameLoader.messageManager;

A comment might be helpful for future generations...
I'm wondering if we should pass a messageManager to connectToChild instead of a frame. That would allow to fetch the message manager only from one place and put a detailed comment about that trick.

::: devtools/server/main.js:1049
(Diff revision 1)
>        dumpn("establishing forwarding for app with prefix " + prefix);
>  
>        actor = msg.json.actor;
>  
>        let { NetworkMonitorManager } = require("devtools/shared/webconsole/network-monitor");
>        netMonitor = new NetworkMonitorManager(frame, actor.actor);

Network monitor is broken, that may be related to this. I imagine we fetch the messageManager via frameLoader.
Attachment #8767365 - Flags: review?(poirot.alex)
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
https://reviewboard.mozilla.org/r/61890/#review58786

You'll want to look at the next version of this, I had to make additional server changes to get the non-e10s case to work correctly.

> Same here. I imagine the title would correctly be retrieved from the child. The only useful thing is handling zombie tabs.

Okay, I removed `this._browser.contentTitle;` part.

> I imagine this currentURI is a relic from the past? Or just some code specific to in-process?
> We should either get a url from the child correctly or from the session store for zombie tabs.

Yes, it seems like it may have been specific to in process.  I don't seem to need it any more in my testing.

> A comment about this title and url would be great.
> This is a special case for Firefox, to guess additional metadata from the parent process for unloaded/zombie tabs.

Okay, comment added.
https://reviewboard.mozilla.org/r/61888/#review58880

> Also I'm seeing various exceptions when toggle RDM on google:
this.webNavigation is null -- browser.xml:902
JavaScript error: resource://gre/modules/RemoteAddonsParent.jsm, line 1086: TypeError: cannot use the given object as a weak map key
JavaScript error: resource:///modules/ReaderParent.jsm, line 82: TypeError: win.gBrowser is undefined

> I also get this exception when toggle RDM off while tools are opened:
JavaScript error: resource://gre/modules/ExtensionContent.jsm, line 958: TypeError: this.globals.get(...) is undefined
(might be related to one of my patches)

These are all errors that appeared during previous RDM patches.  They appear to be harmless so far, but should be investigated, at least to silence them.  We have bug 1273721 for these.

> But the console also breaks when doing that, with the following exception:
Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: this.inner.frameLoader is null

I have addressed this by trying to clean up the mm tunnel more thoroughly.  However, for the moment, you will still be left with a confused toolbox if you open it and close RDM.  This situation will be fixed in bug 1240912 where we properly handle the toolbox following the content when toggling RDM.

> Also, I'm wondering if your tunneling approach is going to scale for multiple viewports...
Don't you think it would be easier to pass a precise frame/messageManager to devtools codebase instead of doing this tunneling work? And especially easier when there will be more than just one frame?

I don't expect the tunneling would be used at all for additional viewports, so it is specific to the first one only.  I don't really have specific design details of multiple viewports in mind yet, as there are still many questions to answer about exactly how they should perform.

It sounds a bit awkward to pass a special messageManager to DevTools since it's all done deep in the guts of the server when browsers are listed.

At the moment, I think I prefer the current approach which allows DevTools to just pull "messageManager" like other browser UI code does.  All of the strangeness about tunneling is contained to the RDM specific module for it.

We'll see, perhaps I'll change my mind later. :)

> Is this really useful/wanted if `name` matches a prefix?

Yes, you always want to add a listener to the outer MM.  For messages that get tunneled from inner to outer, the tunnel listens for them on inner and delivers them to outer, so the actual listener being added here needs to be on the outer MM to hear what it wants.

It also means that the listener continues to work if:

1. Tunnel opens
2. Someone adds a listener
3. Tunnel closes

Their listener is still alive and connected to the outer MM as they would expect it to be.

> Instead of duplicating code from devtools/client/inspector/test/head.js, could you instead share these helpers?!

I did try that originally, but it failed in a strange way...  I later learned the other flatten browser patch in this series had broken the test actor registry stuff (their were no global actors loaded).

Anyway, that issue is now resolved, and I have moved the helpers to a common file.

> Shouldn't this be part of the second patch?

I agree it makes more sense in the other patch, I've moved it.

> A comment might be helpful for future generations...
> I'm wondering if we should pass a messageManager to connectToChild instead of a frame. That would allow to fetch the message manager only from one place and put a detailed comment about that trick.

I realized after our call that actually I do want to still have access to the frame in connectToChild generally.  It's needed to hear the browser swapping event.  (I suppose we could add an observer notification instead to avoid needed the frame for only this reason.)

Anyway, I'll add a comment here about the purpose of this.

> Network monitor is broken, that may be related to this. I imagine we fetch the messageManager via frameLoader.

Well, I tried to cook up ways to stop passing the frame here, but I don't have a great answer yet.  You can get window IDs for each request, but if there are subframes in the content, you need to climb up the windows to check each parent ID.

I'll probably have to do something like in the next bug with swapping (bug 1240912), but for now I did a simpler thing.  I changed topFrame to match if the topFrame we filter on is a parent of the request's topFrame.
Comment on attachment 8767365 [details]
Bug 1240907 - Tunnel toolbox messages to content in RDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61888/diff/1-2/
Comment on attachment 8767366 [details]
Bug 1240907 - Flatten RemoteBrowserTabActor into BrowserTabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61890/diff/1-2/
Comment on attachment 8767366 [details]
Bug 1240907 - Flatten RemoteBrowserTabActor into BrowserTabActor.

You should re-review this one since I had to create new loader in the child script for non-e10s mode.
Attachment #8767366 - Flags: review+ → review?(poirot.alex)
Comment on attachment 8767366 [details]
Bug 1240907 - Flatten RemoteBrowserTabActor into BrowserTabActor.

https://reviewboard.mozilla.org/r/61890/#review60286

::: devtools/server/actors/webbrowser.js:2141
(Diff revision 2)
> -RemoteBrowserTabActor.prototype = {
> +BrowserTabActor.prototype = {
>    connect() {
>      let onDestroy = () => {
>        this._form = null;
>      };
> -    let connect = DebuggerServer.connectToChild(
> +    let connect = DebuggerServer.connectToChild(this._conn, this._browser, onDestroy);

This line looks too long.

::: devtools/server/child.js:15
(Diff revision 2)
>    var chromeGlobal = this;
>  
>    // Encapsulate in its own scope to allows loading this frame script more than once.
>    (function () {
> -    let Cu = Components.utils;
> -    let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +    const Cu = Components.utils;
> +    const { DevToolsLoader } = Cu.import("resource://devtools/shared/Loader.jsm", {});

We have to be cautious about his. It means that any non-module doing `let { require } = Cu.import(Loader.jsm)` will now be spawning yet another third instance in e10s.

I haven't been able to discover any such scenario except: frame-script-utils.js
But given it spawn helper modules that aren't related to server, it shouldn't break. It will just create another loader instance for nothing.

Please verify at runtime that no unexpected loader instances are created in e10s. We should only have two in total and one per process.
Attachment #8767366 - Flags: review?(poirot.alex) → review+
Comment on attachment 8769015 [details]
Bug 1240907 - Clean up code style in test-actor-registry.js.

https://reviewboard.mozilla.org/r/62994/#review60290

Thanks for the cleanup!

::: devtools/client/shared/test/test-actor-registry.js:78
(Diff revision 1)
>      let client = new DebuggerClient(DebuggerServer.connectPipe());
>  
>      yield client.connect();
>  
> -  // We also need to make sure the test actor is registered on the server.
> +    // We also need to make sure the test actor is registered on the server.
> -    yield registerTestActor(client);
> +    yield exports.registerTestActor(client);

What that your main issue?
Surprising it was working...
Attachment #8769015 - Flags: review?(poirot.alex) → review+
Comment on attachment 8767365 [details]
Bug 1240907 - Tunnel toolbox messages to content in RDM.

https://reviewboard.mozilla.org/r/61888/#review60292

I would like to know more about the network monitor tweak before proceeding.

Also I still see this exception when closing Firefox with one tab with RDM on and a toolbox opened.
It would be great to land without such exception.
console.error: 
  Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: this.inner.frameLoader is null
Stack: get innerParentMM@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/responsive.html/browser/tunnel.js:370:5
sendAsyncMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/responsive.html/browser/tunnel.js:478:5
ChildDebuggerTransport.prototype.send@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:748:7
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1629:11
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Line: 370, column: 5

> At the moment, I think I prefer the current approach which allows DevTools to just pull "messageManager" like other browser UI code does.  All of the strangeness about tunneling is contained to the RDM specific module for it.

I mostly want to ensure we are not working around our codebase and keep things simple.
We own /devtools/ and should make whatever tweaks help supporting new RDM.
If we can prevent having to go through the weird tunnel thing, I consider that as being simplier/sanier!
For example, you might be able to hook into gDevtools.showToolbox/gDevtoolsBrowser/TabTarget() to open a toolbox for you viewport frame directly.
I imagine you will have to find similar solutions for multiple viewport support.
So please consider reverting that devtools specifics from tunnel once you get a good picture of multiple viewport.

::: devtools/client/responsive.html/browser/tunnel.js
(Diff revision 2)
> -      Object.defineProperty(outer, "messageManager", {
> -        value: mmTunnel,
> -        writable: false,
> -        configurable: true,
> -        enumerable: true,
> -      });

Good call to move that within the tunnel implementation!

::: devtools/client/responsive.html/browser/tunnel.js:393
(Diff revision 2)
> -    this.innerParentMM.sendAsyncMessage(name, ...args);
> +    Object.defineProperty(this.outer, "messageManager", {
> +      value: this,
> +      writable: false,
> +      configurable: true,
> +      enumerable: true,
> +    });

It isn't related to this patch, but should you really pass `this`? It will expose more than what you expect.
It will also expose your init/destroy/any other non-message manager methods.
You may want to craft a custom object and put the overrides and pass through methods on it.

::: devtools/client/responsive.html/browser/tunnel.js:420
(Diff revision 2)
>        // Workaround bug 449811 to ensure a fresh binding each time through the loop
>        let _method = method;
>        this[_method] = (...args) => {
>          return this.outerParentMM[_method](...args);
>        };
>      }

It looks like a half way fix. It will address call sites that caches the message manager, but won't address cases where we cache the methods directly.
But there doesn't seem to be much caching of loadFrameScript and addMessageListener...

Anyway, did you considered using Proxy API?
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Proxy

::: devtools/client/responsive.html/browser/tunnel.js:447
(Diff revision 2)
> +
> +    // If the message name is part of a prefix we're tunneling, we also need to add the
> +    // tunnel as a inner listener.
> +    if (this.INNER_TO_OUTER_MESSAGE_PREFIXES.some(prefix => name.startsWith(prefix))) {
> +      debug(`Add inner listener for ${name}`);
> +      this.innerParentMM.addMessageListener(name, this);

You may potentialy register more than once listener for this name, but in destroy, remove it only once.
You might be able to use tuneledMessageNames to address that.

::: devtools/client/responsive.html/test/browser/head.js:32
(Diff revision 2)
> +  this);
> +
> +// Import helpers for the inspector that are also shared with others
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/devtools/client/inspector/test/shared-head.js",
> +  this);

Thanks for the copy/pasted code removal!!

::: devtools/server/main.js:986
(Diff revision 2)
>     */
>    connectToChild(connection, frame, onDestroy) {
>      let deferred = SyncPromise.defer();
>  
> -    let mm = frame.frameLoader.messageManager;
> +    // Get messageManager from XUL browser (which might be a specialized tunnel for RDM)
> +    // or else fallback to asking the frameLoader itself.

I imagine you want to copy this comment to webbrowser.js and network-monitor.js

::: devtools/shared/webconsole/network-monitor.js:94
(Diff revision 2)
> +        break;
> +      }
> +      topFrame = topFrame.ownerGlobal
> +                         .QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIDOMWindowUtils)
> +                         .containerElement;

> I changed topFrame to match if the topFrame we filter on is a parent of the request's topFrame.

I don't get that?
getTopFrameForRequest doesn't return the top frame?
What kind of toper frame do we query here?
Shouldn't this be done by getTopFrameForRequest?
`ownerGlobal` is related to EventTarget interface, this is kind of unexpected usage of it.
Wouldn't topFrame.ownerDocument.defaultView be equivalent?
Attachment #8767365 - Flags: review?(poirot.alex)
https://reviewboard.mozilla.org/r/62994/#review60290

> What that your main issue?
> Surprising it was working...

Actually no, I think I just wasn't including the extra test import in the manifest for my directory (which leads to mysterious errors that have no connection to the problem).  But I agree I am not sure how it worked without this before!
https://reviewboard.mozilla.org/r/61890/#review60286

> This line looks too long.

We increased our line length in /devtools to 90 chars recently, it's below that limit.

> We have to be cautious about his. It means that any non-module doing `let { require } = Cu.import(Loader.jsm)` will now be spawning yet another third instance in e10s.
> 
> I haven't been able to discover any such scenario except: frame-script-utils.js
> But given it spawn helper modules that aren't related to server, it shouldn't break. It will just create another loader instance for nothing.
> 
> Please verify at runtime that no unexpected loader instances are created in e10s. We should only have two in total and one per process.

I found a way to make things work without a separate loader, so we should be able to avoid these issues.
https://reviewboard.mozilla.org/r/61888/#review60292

> Also I still see this exception when closing Firefox with one tab with RDM on and a toolbox opened.
It would be great to land without such exception.
console.error: 
  Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: this.inner.frameLoader is null
  
Thanks for pointing this out!  I've added additional checks and cleanup paths to fix this.

> It isn't related to this patch, but should you really pass `this`? It will expose more than what you expect.
> It will also expose your init/destroy/any other non-message manager methods.
> You may want to craft a custom object and put the overrides and pass through methods on it.

Hmm, yes, perhaps.  It would be nice to let XPCOM hide it for us, but it's not just a single interface and people don't use QueryInterface with it, as it's some magic DOM multi-interface thing[1].

But anyway, yes, we could make a separate object with just the things to expose.  Filed bug 1286146.

[1]: https://dxr.mozilla.org/mozilla-central/rev/214884d507ee369c1cf14edb26527c4f9a97bf48/dom/base/nsDOMClassInfo.cpp#598

> It looks like a half way fix. It will address call sites that caches the message manager, but won't address cases where we cache the methods directly.
> But there doesn't seem to be much caching of loadFrameScript and addMessageListener...
> 
> Anyway, did you considered using Proxy API?
> https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Proxy

That's true, it would miss cases where specific methods are cached.  In my searching, I haven't see such cases, so I am not too worried for the moment.

I did try the Proxy API earlier on for other XUL browser properties and I could not find a good way to make it fit.  I ran into many errors with platform code paths that complained I did not implement the right interfaces, etc.  I did not see a way to fix it from JS only, so I ended up down the path we have here.

I agree Proxy is a better conceptual fit, but I don't think it works well for platform APIs.

> You may potentialy register more than once listener for this name, but in destroy, remove it only once.
> You might be able to use tuneledMessageNames to address that.

Adding more than once should be okay, the impl of addMessageListener checks first to see if the listener already exists[1].  But it is true that the sequence "add, add, remove" would remove the tunnel listener too early.

I could track usage counts, but that's more complexity than seems worth it here.  Instead, I think I'll stop removing the tunnel listeners in removeMessageListener and leave them all until the tunnel is destroyed.

[1]: https://dxr.mozilla.org/mozilla-central/rev/214884d507ee369c1cf14edb26527c4f9a97bf48/dom/base/nsFrameMessageManager.cpp#389

> I imagine you want to copy this comment to webbrowser.js and network-monitor.js

Okay, I've added the comment in those files too.

> > I changed topFrame to match if the topFrame we filter on is a parent of the request's topFrame.
> 
> I don't get that?
> getTopFrameForRequest doesn't return the top frame?
> What kind of toper frame do we query here?
> Shouldn't this be done by getTopFrameForRequest?
> `ownerGlobal` is related to EventTarget interface, this is kind of unexpected usage of it.
> Wouldn't topFrame.ownerDocument.defaultView be equivalent?

As we dicussed over Vidyo, we'll keep this topFrame hack for the moment, but it should be removed quite soon in the next bug 1240912.

I've added a comment to describe the situation.
Comment on attachment 8767365 [details]
Bug 1240907 - Tunnel toolbox messages to content in RDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61888/diff/2-3/
Attachment #8767365 - Flags: review?(poirot.alex)
Comment on attachment 8767366 [details]
Bug 1240907 - Flatten RemoteBrowserTabActor into BrowserTabActor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61890/diff/2-3/
(I still need to fix up more tests for the non-e10s case... but let's see how we're doing.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cd929f40bb1
Comment on attachment 8767365 [details]
Bug 1240907 - Tunnel toolbox messages to content in RDM.

https://reviewboard.mozilla.org/r/61888/#review60592

Works great. Do not have the reported expection on close.
I feel like something could be better on the destroy/cleanup codepath.
Hopefully that would help handling races...

::: devtools/client/responsive.html/browser/tunnel.js:446
(Diff revisions 2 - 3)
> +      return;
> +    }
> +    if (subject == this.innerParentMM) {
> +      debug("Inner messageManager has closed");
> +      this.destroy();
> +    }

Just wondering. Does the inner message manager closes always before the outer one?
If not, it could be worth also cleaning things up when the outer get closed.

::: devtools/client/responsive.html/browser/tunnel.js:486
(Diff revisions 2 - 3)
>      debug(`Remove outer listener for ${name}`);
>      // Remove an outer listener, just like a simple pass through
>      this.outerParentMM.removeMessageListener(name, ...args);
>  
> -    // If the message name is part of a prefix we're tunneling, we also need to remove the
> -    // tunnel as a inner listener.
> +    // Leave the tunnel as an inner listener for the case of prefix messages to avoid
> +    // tracking counts of add calls.  The inner listener will get removed on destroy.

Didn't you meant "Leave the inner listeners..." ?

::: devtools/shared/webconsole/network-monitor.js:102
(Diff revisions 2 - 3)
>          break;
>        }
>        topFrame = topFrame.ownerGlobal
>                           .QueryInterface(Ci.nsIInterfaceRequestor)
>                           .getInterface(Ci.nsIDOMWindowUtils)
>                           .containerElement;

Did you tried topFrame.ownerGlobal.frameElement?

Otherwise instead of doing a while() loop, there is this helper which does what you want, but requires docshell object:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/layout/utils.js#101

::: devtools/client/responsive.html/browser/tunnel.js:396
(Diff revision 3)
> -      debug(`Should ${name} go to inner?`);
> -      this.outerParentMM.sendAsyncMessage(name, ...args);
> +      this.innerParentMM.addMessageListener(name, this);
> +      this.tunneledMessageNames.add(name);
> -      return;
>      }
>  
> -    debug(`${name} outer -> inner`);
> +    Services.obs.addObserver(this, "message-manager-close", false);

I would be great to comment about why you need to cleanup things early on inner message manager close.

tunnel.destroy should be called right after by tunnelToInnerBrowser.stop() It looks like it is being call on "beforeunload" event.

I'm wondering if we should destroy things earlier from the highligher level abstractions?
We are introducing the possibility to destroy the MessageManagerTunnel instance while keeping all the upper level abstraction alive and most likely half broken.
Attachment #8767365 - Flags: review?(poirot.alex) → review+
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
https://reviewboard.mozilla.org/r/61888/#review60592

> Just wondering. Does the inner message manager closes always before the outer one?
> If not, it could be worth also cleaning things up when the outer get closed.

Actually, it seems like outer one closes before the inner when closing tab.

I added a check for the outer one as well, so we'll destroy when either mm is closed.

> Didn't you meant "Leave the inner listeners..." ?

Hmm, I'm not sure what you mean?  We add the tunnel as an inner listener for each prefix message name.  We leave it in place here and clean up each on on destroy.  Maybe it was about singular vs. plural...?  But I think it's right...  Maybe I've been looking at the computer for too long. :)

> Did you tried topFrame.ownerGlobal.frameElement?
> 
> Otherwise instead of doing a while() loop, there is this helper which does what you want, but requires docshell object:
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/layout/utils.js#101

Yes, I tried `topFrame.ownerGlobal.frameElement`, but it does not appear to cross mozbrowser boundaries.

I also tried layout/utils early on, but I don't have the docshell inside the frame since it's remote.

> I would be great to comment about why you need to cleanup things early on inner message manager close.
> 
> tunnel.destroy should be called right after by tunnelToInnerBrowser.stop() It looks like it is being call on "beforeunload" event.
> 
> I'm wondering if we should destroy things earlier from the highligher level abstractions?
> We are introducing the possibility to destroy the MessageManagerTunnel instance while keeping all the upper level abstraction alive and most likely half broken.

Okay, I've added a comment.  Checking MM close is only important for cases like browser window close and application exit.

However, you do bring up a great point about "beforeunload"!  I discovered that after the initial message tunnel was added, the "beforeunload" code paths stopped working, because "beforeunload" was now going to the content browser insstead of the RDM UI.

As it turns out, now that we are tunneling messages, we don't need to watch for "beforeunload" to patch up SessionStore on browser close.  So, I was able to remove that part.  However, we do still want some way to shut down RDM when a tab cloes.  So, I am now watching for TabClose events.

Thanks for noticing this!
Last try was all green, here's a combined version with review fixes as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a8e75b46f7f
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1345f3fb280
Clean up code style in test-actor-registry.js. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8dcecb1933
Tunnel toolbox messages to content in RDM. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/88d6dfa406b8
Flatten RemoteBrowserTabActor into BrowserTabActor. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/020168373810
Log server destroy errors. r=me
I managed to reproduce this issue on Firefox 49.0a1 (2016-05-28) and on Windows 10 x64.
I've tested around this bug on Firefox 50.0a1 (2016-07-29) and encountered a possible issue - the elements from the page can not be inspected while the RDM is enabled. Is this behavior expected for now?
QA Whiteboard: [qe-rdm]
Flags: needinfo?(jryans)
(In reply to Mihai Boldan, QA [:mboldan] from comment #34)
> I've tested around this bug on Firefox 50.0a1 (2016-07-29) and encountered a
> possible issue - the elements from the page can not be inspected while the
> RDM is enabled. Is this behavior expected for now?

What do you mean by "the elements of the page" here, which page?  The main result of this bug is that the DevTools will now inspect the page content inside the viewport, instead of the outer RDM UI.  If you mean that you can no longer inspect the RDM UI, then yes, that's expected.  The page content inside the viewport is meant to be inspectable.

However, currently the toolbox will still close or become broken if you open the toolbox before entering RDM.  We have bug 1240912 to resolve this.  So, you would need to first open RDM and then open the toolbox after that.
Flags: needinfo?(jryans)
Since I didn't found any other issues beside Bug 1292133, I am marking this issue Verified Fixed.
The tests were performed on Firefox 51.0a1 (2016-08-03) and on Windows 10 x64, Mac OS X 10.11.1 and on Ubuntu 16.04 x64
Status: RESOLVED → VERIFIED
status-firefox51: --- → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: