Closed Bug 1240912 Opened 8 years ago Closed 8 years ago

Toolbox should remain open when toggling RDM

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox51 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

Details

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

Attachments

(9 files)

Bug 1240912 - Beacon filtering no longer needed.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Filter requests by top frame's outerWindowID.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Toolbox remains connected across frame swaps.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - NetworkMonitorManager uses mm instead of frame.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Change storage actor messages to use debug prefix.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Support sync messages inner to outer in the tunnel.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Convert Network Monitor to setupInParent.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Add browser swap support to multiprocess actors.
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1240912 - Add outerWindowID getter for request matching.
58 bytes, text/x-review-board-request
ochameau
: review+
Details 
The basic shell from bug 1239437 does not allow the toolbox to stay open when the toggling RDM on and off.

This is because RDM changes the tab to a new chrome page, and the page content is actually a sub-frame of this.

It may be quite challenging to keep the same toolbox alive since we are moving the page content in this way, but we should at a minimum reload the toolbox connected to the content.  If it's possible to actually keep the same toolbox open, then we should do so, since that offers the best UX.
Priority: -- → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Priority: P2 → P1
QA Contact: mihai.boldan
Can you provide any details on the approach that should be taken or points for investigations?
Flags: needinfo?(jryans)
Priority: P1 → P2
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #1)
> Can you provide any details on the approach that should be taken or points
> for investigations?

I think this is somewhat blocked on my current work, so I've asked Marco to move it out of the current release and replace it with bug 970346 instead.

As far as implementation, once we are swapping page content (bug 1240913), we should investigate what events are causing the toolbox to close.  It could be the step where the remoteness of the page changes, since we currently close the toolbox if you try that today (as in going between web content and about:addons or something).  We should be able to make an exception for the case where content is swapped between frames.  This might involve tweaking the webbrowser actor to support this case.  I am unsure of the exact cause so far.  I don't have too many details yet, since I think the first step will be pinpointing which step in the RDM process is forcing the toolbox to close.

My guess is that the fix for this and for bug 1240907 maybe be related / the same.

I have asked Marco to move this out of the current release since it's probably blocked on me.
Depends on: 1240913
Flags: needinfo?(jryans)
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
Should be able to re-enable browser_resize_cmd.js as part of this work.
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Comment on attachment 8779117 [details]
Bug 1240912 - Beacon filtering no longer needed.

https://reviewboard.mozilla.org/r/70136/#review67616
Attachment #8779117 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review67596

Hum. I suspect this to be subject to race conditions.
Do we really have to switch to match against window ids?
Frames is safier, there is always a delay on the parent side to receive the related window IDs.

::: devtools/server/actors/webbrowser.js:876
(Diff revision 1)
>    // Flag eventually overloaded by sub classes in order to watch new docshells
>    // Used on b2g to catch activity frames and in chrome to list all frames
> -  this.listenForNewDocShells =
> -    Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT;
> +  // TODO: When should this actually be true?  We want false for content process browser
> +  // tabs, at least...  If it's true with a browser tab, we start watching *all* docshells
> +  // from any new tab!
> +  this.listenForNewDocShells = false;

Yes... this is very specific to b2g. For regular tabs, we receive every interesting frames from the web progress listener. This isn't the case on b2g where there is some other frames that are not under the same docshell hierarchy.

We should either keep it for b2g or rip it off completely. It is still used for the chrome actor. I'm wondering if we can just simplify the comment here, or move this watching code to chrome.js?
Comment on attachment 8779119 [details]
Bug 1240912 - Toolbox remains connected across frame swaps.

https://reviewboard.mozilla.org/r/70140/#review67594

::: devtools/client/responsive.html/browser/swap.js:145
(Diff revision 1)
>        gBrowser.updateBrowserRemoteness(tab.linkedBrowser, true);
>  
>        // 6. Swap the content into the original browser tab and close the
>        //    temporary tab used to hold the content via
>        //    `swapBrowsersAndCloseOther`.
> +      dispatchDevToolsBrowserSwap(contentBrowser, tab.linkedBrowser);

Can't we send only one devtools swap event?
from innerBrowser to tab.linkedBrowser?

::: devtools/server/main.js:1064
(Diff revision 1)
> +      frame.removeEventListener("devtools:browserswap", onBrowserSwap);
> +      mm.removeMessageListener("debug:setup-in-parent", onSetupInParent);
> +      if (!actor) {
> +        mm.removeMessageListener("debug:actor", onActorCreated);
> +      }
> +      DebuggerServer._childMessageManagers.delete(mm);

We should factorize these message listener addition/removal with regular codepath.

::: devtools/shared/transport/transport.js:780
(Diff revision 1)
> +      if (this._mm) {
> +        this._mm.removeMessageListener(this._messageName, this);
> -    }
> +      }
> +      this._mm = mm;
> +      this._mm.addMessageListener(this._messageName, this);
> +    },

I would have reused existing code:
swapBrowser(mm) {
  this.cleanup();
  this._mm = mm;
  this.ready();
}
And factorize cleanup() with close(), where both swapBrowser and destroy do the same call to removeMessageListener
Comment on attachment 8779120 [details]
Bug 1240912 - NetworkMonitorManager uses mm instead of frame.

https://reviewboard.mozilla.org/r/70142/#review67624
Attachment #8779120 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779121 [details]
Bug 1240912 - Change storage actor messages to use debug prefix.

https://reviewboard.mozilla.org/r/70144/#review67628
Attachment #8779121 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779122 [details]
Bug 1240912 - Support sync messages inner to outer in the tunnel.

https://reviewboard.mozilla.org/r/70146/#review67632
Comment on attachment 8779122 [details]
Bug 1240912 - Support sync messages inner to outer in the tunnel.

https://reviewboard.mozilla.org/r/70146/#review67634
Attachment #8779122 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779123 [details]
Bug 1240912 - Convert Network Monitor to setupInParent.

https://reviewboard.mozilla.org/r/70148/#review67612

::: devtools/server/main.js:993
(Diff revision 1)
>      this._childMessageManagers.add(mm);
>  
>      let actor, childTransport;
>      let prefix = connection.allocID("child");
> -    let netMonitor = null;
> +    // Compute the same prefix that's used by DebuggerServerConnection
> +    let connPrefix = prefix + "/";

nit: I'm not ultimately convinced it is easier to follow by using two different prefixes.
Comment on attachment 8779124 [details]
Bug 1240912 - Add browser swap support to multiprocess actors.

https://reviewboard.mozilla.org/r/70150/#review67602

::: devtools/server/docs/actor-e10s-handling.md:86
(Diff revision 1)
> -    gTrackedMessageManager.delete(mm);
> +  function handleMessageManagerDisconnected() {
> +    DebuggerServer.off("browser-swap:" + prefix, handleBrowserSwap);
>  
> -    // unregister for director-script requests handlers from the parent process (if any)
> -    mm.removeMessageListener("debug:director-registry-request", handleChildRequest);
> +    // Stop listening for messages from the actor in the child process.
> +    mm.removeMessageListener("debug:some-message-name", handleChildRequest);
> +    mm = null;

It becomes complex and complexier, whereas this setup in parent process is already complex enough :o

With this new thing, we have to duplicate in every setup in parent function the addMessageListener+removeEventListener.

We also have to manually call browser-swap:prefix in addition to disconnected-from-child.

We could introduce some helpers to simplify all that.
Here is a potential simplification:

exports.setupParentProcess = function (helper) {
  helper.onMessageManagerCreated(mm => {
    this.mm = mm;
    mm.addMessageListener(...)
  });
  helper.onMessageManagerDestroyed(mm => {
    this.mm = null
    mm.removeMessageListener(...)
  });
}

(Note that I don't care about the API itself much, I would just like to simplify all call sites)
Attachment #8779124 - Flags: review?(poirot.alex)
Comment on attachment 8779123 [details]
Bug 1240912 - Convert Network Monitor to setupInParent.

https://reviewboard.mozilla.org/r/70148/#review67644
Attachment #8779123 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review67646
Attachment #8779118 - Flags: review?(poirot.alex)
Comment on attachment 8779119 [details]
Bug 1240912 - Toolbox remains connected across frame swaps.

https://reviewboard.mozilla.org/r/70140/#review67648
Attachment #8779119 - Flags: review?(poirot.alex)
Overall these patches work great!
I haven't been able to see any breakage, nor any weird error in logs.
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review67596

The main concern that I can see is what happens when subdocuments are loaded (like an iframe on a page).  I've done some more tests with current approach locally.  The request for an iframe's document is tagged like all others on a top level document, meaning it will have the top level window as its outerWindowID.  All of the resources inside the iframe's document are tagged with a new outerWindowID for the iframe window and have the top level window as the parentOuterWindowID.

In the first version you reviewed here, we were actually missing the additional resources on the subdocument because we did not have the new subwindow ID in time.  I have changed the approach to also check the parent window ID as well.

I added a new network monitor test with an iframe so we can have more confidence about this.  Something I found interesting when testing on a public sample page I made (https://output.jsbin.com/depeso) is that this new window ID approach catches requests to Google Analytics that are missed in Nightly today.

Later patches in this series are simpler because we're able to remove dependence on the frame here.  We don't have to use window IDs if there is a better way.  Any suggestions?

> Yes... this is very specific to b2g. For regular tabs, we receive every interesting frames from the web progress listener. This isn't the case on b2g where there is some other frames that are not under the same docshell hierarchy.
> 
> We should either keep it for b2g or rip it off completely. It is still used for the chrome actor. I'm wondering if we can just simplify the comment here, or move this watching code to chrome.js?

My assumption is we don't care of b2g-specifics anymore.  The window actor for Positron (that I still need merge in bug 1268134) might or might not want this behavior.  So, I think I'll set it to false and update the comment.
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review67596

I also needed to update one Style Editor test that was using the Network Monitor to see if its own fetch was loaded using the cache.  With the window ID approach, this internal DevTools request doesn't appear in the Network Monitor, and I think that's okay, so I modified the test.
Comment on attachment 8779119 [details]
Bug 1240912 - Toolbox remains connected across frame swaps.

https://reviewboard.mozilla.org/r/70140/#review67594

> Can't we send only one devtools swap event?
> from innerBrowser to tab.linkedBrowser?

I tried several variations of this, but they all resulted in the toolbox closing.  In the start case, only one event is needed because the page content itself only swaps once.  (The second swap during opening is swapping the RDM UI around the page.)  In the stop case, the page is being move in both swaps, so it seems logical to have two separate events.

> We should factorize these message listener addition/removal with regular codepath.

Okay, I've moved these bits to functions and removed the duplication.

> I would have reused existing code:
> swapBrowser(mm) {
>   this.cleanup();
>   this._mm = mm;
>   this.ready();
> }
> And factorize cleanup() with close(), where both swapBrowser and destroy do the same call to removeMessageListener

Okay, I moved these to functions as well.
Comment on attachment 8779123 [details]
Bug 1240912 - Convert Network Monitor to setupInParent.

https://reviewboard.mozilla.org/r/70148/#review67612

> nit: I'm not ultimately convinced it is easier to follow by using two different prefixes.

Well, I agree actually...  I only did this because I needed a prefix that is reachable from inside an actor (`this.conn.prefix`) which also matches the one used by the `browser-swap` setup in parent message.  The prefix on the connection uses the slash...  I started to look at the connection's prefix and _remove_ the slash from it, but that seemed too complex at the time.

I can try harder if you feel strongly about reducing it...
Comment on attachment 8779124 [details]
Bug 1240912 - Add browser swap support to multiprocess actors.

https://reviewboard.mozilla.org/r/70150/#review67602

> It becomes complex and complexier, whereas this setup in parent process is already complex enough :o
> 
> With this new thing, we have to duplicate in every setup in parent function the addMessageListener+removeEventListener.
> 
> We also have to manually call browser-swap:prefix in addition to disconnected-from-child.
> 
> We could introduce some helpers to simplify all that.
> Here is a potential simplification:
> 
> exports.setupParentProcess = function (helper) {
>   helper.onMessageManagerCreated(mm => {
>     this.mm = mm;
>     mm.addMessageListener(...)
>   });
>   helper.onMessageManagerDestroyed(mm => {
>     this.mm = null
>     mm.removeMessageListener(...)
>   });
> }
> 
> (Note that I don't care about the API itself much, I would just like to simplify all call sites)

I agree it is getting pretty complex...  I made an attempt at something like what you are suggesting, but I am not sure if I like it so far.  One unfortunate part is that I think we have to maintain the existing disconnected event to support existing add-ons.  I chose to expose it as an `onBrowserSwap` and `onDisconnected` set of helpers because I think it's important for the browser swap to start and finish within a single method as far the module is concerned, so that it feels like an atomic transaction and it always has some valid message maanger.

Anyway, I'll post my attempt as an extra patch at the end of this series.  If it looks good to you, I'll squash it into this one when landing.
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
We should verify the server notices messageManager close during browser_shutdown_close_sync.js with these changes.  This was noticed as an issue over in bug 1285566.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> Later patches in this series are simpler because we're able to remove
> dependence on the frame here.  We don't have to use window IDs if there is a
> better way.  Any suggestions?

Keep using frame? That's my first reaction.

But I see window IDs being more and more used within e10s codebase.
Here, we are a bit extreme about these IDs. I imagine we could simplify that by passing only one ID between the TabActor living in the child and the network monitor code in the parent.
The top frame window ID. At least if there is some race condition, it would be only for early requests from the top document. Then, network monitor code in the parent could keep using the topFrameElement or equivalents.

In NetworkMonitorChild.init, you could pass the top frame window id via the `action: "start"` message.
Then in matchRequest you could keep the existing code with the while(topFrame) loop but instead of matches frames against frames you would match window ID against frames window IDs.
I imagine it would not allow you to see the additional google analytics requests, but we should figure out that separately.

Doing that would make less cross process messages and should reduce the possible races.

> My assumption is we don't care of b2g-specifics anymore.  The window actor
> for Positron (that I still need merge in bug 1268134) might or might not
> want this behavior.  So, I think I'll set it to false and update the comment.

Good call.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29)
> I started to look at the connection's prefix
> and _remove_ the slash from it, but that seemed too complex at the time.
> 
> I can try harder if you feel strongly about reducing it...

It would be great to unify the prefixes, but not a blocker for this.
I would be happy to review such simplication afterward!
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #30)
> Comment on attachment 8779124 [details]
> Bug 1240912 - Add browser swap support to multiprocess actors.
> 
> https://reviewboard.mozilla.org/r/70150/#review67602
> 
> > It becomes complex and complexier, whereas this setup in parent process is already complex enough :o
> > 
> > With this new thing, we have to duplicate in every setup in parent function the addMessageListener+removeEventListener.
> > 
> > We also have to manually call browser-swap:prefix in addition to disconnected-from-child.
> > 
> > We could introduce some helpers to simplify all that.
> > Here is a potential simplification:
> > 
> > exports.setupParentProcess = function (helper) {
> >   helper.onMessageManagerCreated(mm => {
> >     this.mm = mm;
> >     mm.addMessageListener(...)
> >   });
> >   helper.onMessageManagerDestroyed(mm => {
> >     this.mm = null
> >     mm.removeMessageListener(...)
> >   });
> > }
> > 
> > (Note that I don't care about the API itself much, I would just like to simplify all call sites)
> 
> I agree it is getting pretty complex...  I made an attempt at something like
> what you are suggesting, but I am not sure if I like it so far.  One
> unfortunate part is that I think we have to maintain the existing
> disconnected event to support existing add-ons.

That's not a big deal to keep sending this event.

> I chose to expose it as an
> `onBrowserSwap` and `onDisconnected`

The overall way you expose these helpers looks good.

> set of helpers because I think it's
> important for the browser swap to start and finish within a single method as
> far the module is concerned, so that it feels like an atomic transaction and
> it always has some valid message manager.

Do we? Do the modules care to handle swap explicitely and atomicely?
The caller, the browser swapping code expects that, I'm not sure the modules, the dynamic actors do.
I'm all but against such API. But I don't see the benefits I'm expecting.
What I really dislike is the duplication of the addMessageListener/removeMessageListener, this is for sure something that is going to bite us. Anytime we will add a new message, we are most likely going to miss adding it for swap codepath.
We could factorize add and remove calls in each actor, manually, but I wish the API would help us doing that.
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review69908

::: devtools/client/netmonitor/test/browser_net_frame.js:151
(Diff revision 2)
> +  },
> +];
> +
> +const REQUEST_COUNT = EXPECTED_REQUESTS_TOP.length + EXPECTED_REQUESTS_SUB.length;
> +
> +var test = Task.async(function* () {

nit:
  add_task(function* () {
?

::: devtools/client/netmonitor/test/browser_net_frame.js:163
(Diff revision 2)
> +  let [, debuggee, monitor] = yield initNetMonitor(SIMPLE_URL);
> +  let { NetMonitorView } = monitor.panelWin;
> +  let { RequestsMenu } = NetMonitorView;
> +  RequestsMenu.lazyUpdate = false;
> +
> +  debuggee.location = TOP_URL;

nit: may be use ContentTask or some other helper to not use CPOW.

::: devtools/client/netmonitor/test/browser_net_frame.js:220
(Diff revision 2)
> +      is(stackLen, 0, `Request #${i} (${causeType}) has an empty stacktrace`);
> +    }
> +  }
> +
> +  yield teardown(monitor);
> +  finish();

calling finish() is optional when using add_task
Attachment #8779118 - Flags: review?(poirot.alex)
Comment on attachment 8779119 [details]
Bug 1240912 - Toolbox remains connected across frame swaps.

https://reviewboard.mozilla.org/r/70140/#review69910

r+ for the vast majority of this patch.
I'm still not confident about the swap/disconnected API, but you are focusing on that in another patch.
Attachment #8779119 - Flags: review?(poirot.alex) → review+
Brief summary about the patches and the review.

Attachment 8779118 [details], tell me if using only top frame window ID works for you.
All the other attachment, attachment 8779119 [details], attachment 8779124 [details] and attachment 8779941 [details] are all related to the API and comment 43.
(In reply to Alexandre Poirot [:ochameau] from comment #41)
> But I see window IDs being more and more used within e10s codebase.
> Here, we are a bit extreme about these IDs. I imagine we could simplify that
> by passing only one ID between the TabActor living in the child and the
> network monitor code in the parent.
> The top frame window ID. At least if there is some race condition, it would
> be only for early requests from the top document. Then, network monitor code
> in the parent could keep using the topFrameElement or equivalents.
> 
> In NetworkMonitorChild.init, you could pass the top frame window id via the
> `action: "start"` message.
> Then in matchRequest you could keep the existing code with the
> while(topFrame) loop but instead of matches frames against frames you would
> match window ID against frames window IDs.
> I imagine it would not allow you to see the additional google analytics
> requests, but we should figure out that separately.

I am not sure I understand how this would work for nested iframes within the content page.  A request for a resource inside a deeply nested frame doesn't have the top frame's window ID on it.  If we only have the top frame's window ID in network monitor, how do you decide the match?

You also mention topFrameElement, so I am not sure if you are implying some hybrid of both window ID and frame element together.  Can you be more specific?
Flags: needinfo?(poirot.alex)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #47)
> I am not sure I understand how this would work for nested iframes within the
> content page.  A request for a resource inside a deeply nested frame doesn't
> have the top frame's window ID on it.  If we only have the top frame's
> window ID in network monitor, how do you decide the match?

loadInfo has outerWindowID and parentOuterWindowID,
but using topFrameElement (that we are using today), you should be able to retrieve, for a given request, the related top frame window ID.

> 
> You also mention topFrameElement, so I am not sure if you are implying some
> hybrid of both window ID and frame element together.  Can you be more
> specific?

It is hybrid but only within matchRequest. It still allows you to remove frame usage from NetworkMonitorManager/NetworkMonitorParent.
Flags: needinfo?(poirot.alex)
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review69908

Your approach of using the top frame's window ID appears to be working well, thanks for the idea! :)

> nit:
>   add_task(function* () {
> ?

Makes sense!  (I borrowed a lot of this from browser_net_cause.js.)

> nit: may be use ContentTask or some other helper to not use CPOW.

Changed to calling `loadURI` on the browser.

> calling finish() is optional when using add_task

Thanks, removed.
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review70160

::: devtools/server/actors/webbrowser.js:1886
(Diff revision 4)
>      Object.defineProperty(this, "docShell", {
>        value: docShell,
>        enumerable: true,
>        configurable: true
>      });
>      events.emit(this, "changed-toplevel-document");

Note that the netmonitor doesn't support that so far, but you could listen for this event from NetmonitorChild in order to listen only for a given sub frame. Your previous patch looked like it was supporting listening only for a given sub document requests.
It looks like a good followup.

::: devtools/shared/webconsole/network-monitor.js:1651
(Diff revision 4)
>      if (this.messageManager) {
> -      this.messageManager.removeMessageListener("debug:netmonitor:" + this.id,
> +      let mm = this.messageManager;
> -                                                this.onNetMonitorMessage);
> -    }
> -    this.messageManager = null;
> +      this.messageManager = null;
> -    this.filters = null;
> +      mm.removeMessageListener("debug:netmonitor:" + this.id, this.onNetMonitorMessage);

Just wondering why you nullify messageManager first?

::: devtools/shared/webconsole/network-monitor.js:1653
(Diff revision 4)
> -                                                this.onNetMonitorMessage);
> -    }
> -    this.messageManager = null;
> +      this.messageManager = null;
> -    this.filters = null;
> +      mm.removeMessageListener("debug:netmonitor:" + this.id, this.onNetMonitorMessage);
> +    }
> +    this.frames = null;

This is a left over.
Attachment #8779118 - Flags: review?(poirot.alex)
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review70162

So much better, thanks for the simplication!
Attachment #8779118 - Flags: review+
Comment on attachment 8779124 [details]
Bug 1240912 - Add browser swap support to multiprocess actors.

https://reviewboard.mozilla.org/r/70150/#review70164

Thanks for this second simplication, that looks great to me.
There is just this "swapBrowser" function used everywhere that could benefit from a better name.

::: devtools/server/docs/actor-e10s-handling.md:81
(Diff revision 4)
> -    mm.removeMessageListener("debug:director-registry-request", handleChildRequest);
>    }
> +
> +  return {
> +    onBrowserSwap: swapBrowser,
> +    onDisconnected: () => swapBrowser(null),

This is weird having onDisconnected calling something called swapBrowser.
What about this naming?
return {
  onBrowserSwap: setBrowser,
  onDisconnect: () => setBrowser(null)
}

Or updateBrowser, setMessageManager, updateMessageManager or anything more generic?
Attachment #8779124 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779941 [details]
Bug 1240912 - Add outerWindowID getter for request matching.

https://reviewboard.mozilla.org/r/70814/#review70168

::: devtools/client/responsive.html/browser/swap.js:223
(Diff revision 3)
> +        return browser._outerWindowID;
> +      },
> +      configurable: true,
> +      enumerable: true,
> +    });
> +  }

Hum. That means the network monitor won't work with plain <iframe mozbrowser>. But that's not really your business here.
If you have ideas on how to support them, that would be great...
Attachment #8779941 - Flags: review?(poirot.alex) → review+
Comment on attachment 8779118 [details]
Bug 1240912 - Filter requests by top frame's outerWindowID.

https://reviewboard.mozilla.org/r/70138/#review70160

> Note that the netmonitor doesn't support that so far, but you could listen for this event from NetmonitorChild in order to listen only for a given sub frame. Your previous patch looked like it was supporting listening only for a given sub document requests.
> It looks like a good followup.

Yes, it should be possible to listen to only a sub frame.  A good idea for future work!

> Just wondering why you nullify messageManager first?

I don't think there's really a reason for this anymore, it's left over from other changes.  I'll revert it.

> This is a left over.

Thanks, removed.
Comment on attachment 8779124 [details]
Bug 1240912 - Add browser swap support to multiprocess actors.

https://reviewboard.mozilla.org/r/70150/#review70164

> This is weird having onDisconnected calling something called swapBrowser.
> What about this naming?
> return {
>   onBrowserSwap: setBrowser,
>   onDisconnect: () => setBrowser(null)
> }
> 
> Or updateBrowser, setMessageManager, updateMessageManager or anything more generic?

Yeah, it did fell a bit strange as I wrote it.... I changed to `setMessageManager` since that seems the most literal description of what's happening.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40)
> We should verify the server notices messageManager close during
> browser_shutdown_close_sync.js with these changes.  This was noticed as an
> issue over in bug 1285566.

This is still an issue after the patches here.  I filed bug 1296463 to follow up on this.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da4ba32303c5
Beacon filtering no longer needed. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c2e9939e19
Filter requests by top frame's outerWindowID. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f9842022c6
Toolbox remains connected across frame swaps. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a20cc4197d
NetworkMonitorManager uses mm instead of frame. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d8866ee39c
Change storage actor messages to use debug prefix. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e4433ce25f
Support sync messages inner to outer in the tunnel. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c978a14d53a
Convert Network Monitor to setupInParent. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9c0587ffc6
Add browser swap support to multiprocess actors. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/23bc7e76f9ad
Add outerWindowID getter for request matching. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/54fcd475235c
Only remove disconnect listener when closing the child. r=me
The issue is no longer reproducible on Firefox 51.0a1 (2016-08-22), under Windows 10 x64, Mac OS X 10.9.5 and under Ubuntu 16.04 x64.
I am marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-rdm]
status-firefox51: fixed → verified
Flags: qe-verify+
This mechanism does not seem to be working with multiple content process and its test fails. Is that expected?
Flags: needinfo?(jryans)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #78)
> This mechanism does not seem to be working with multiple content process and
> its test fails. Is that expected?

Hmm, I guess I would need to investigate the root cause in more detail first.  I attempted to create this situation locally:

1. Set "dom.ipc.processCount" to 10
2. Restart Firefox
3. Open some page in a tab
4. Open new RDM (devtools.responsive.html.enabled set to true first)
5. Open DevTools toolbox
6. Should be able to freely toggle RDM off and on with toolbox still functioning

I was still able to use the toolbox when toggling RDM open and closed, so at first glance it appeared to still work for me.

Can you provide more details on the steps you were using?

I do see a few test failures in browser_toolbox_swap_browsers.js when I run:

./mach mochitest devtools/client/responsive.html --setpref dom.ipc.processCount=10

Is that a good way to run tests with multiple content processes, or do I need different settings?
Flags: needinfo?(jryans) → needinfo?(gkrizsanits)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #79)
> I was still able to use the toolbox when toggling RDM open and closed, so at
> first glance it appeared to still work for me.

That's great news, many thanks for the quick response. It means that this is more of a testing issue than a bug that we should be afraid of then.

> 
> Can you provide more details on the steps you were using?
> 
> I do see a few test failures in browser_toolbox_swap_browsers.js when I run:
> 
> ./mach mochitest devtools/client/responsive.html --setpref
> dom.ipc.processCount=10
> 
> Is that a good way to run tests with multiple content processes, or do I
> need different settings?

Yes, that's the test I've seen failing and your method is correct. I usually change the pref in all.js but it really should not matter, command line is just as good.
Flags: needinfo?(gkrizsanits)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #79)
> I do see a few test failures in browser_toolbox_swap_browsers.js when I run:
> 
> ./mach mochitest devtools/client/responsive.html --setpref
> dom.ipc.processCount=10

I filed bug 1310346 for this e10s-multi issue.
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: