Mihai said in bug 916166: > 1. network requests happen in the main process, not in content processes > where the content actors work (including the console actor). So, we don't > see any requests. > > 2. network requests in the main process have no associatedWindow property. > The global console actor will show all network requests, from the entire > system, without being able to filter them based on windows or apps. > > We need a solution to have the toolbox connect to the global console actor > and get the network requests for the app that the toolbox is attached to. Or > we needed the content console actor to talk to the main process to get the > network requests associated to the content process. ( i prefer the latter > approach )

The network monitor [1] does not work with FxOS, see comment #0 for details. How should we proceed? Can we get network requests to have an associatedProcess of some sorts? Or do you have any suggestions for how we could get network requests specific to a content process? Thanks! [1] current implementation lives in toolkit/devtools/webconsole/utils.js, see NetworkMonitor. You may also want to see the WebConsoleActor and the NetworkEventActor in toolkit/devtools/server/actors/webconsole.js.

The tight thing to do is presumably to add whatever identifying info is needed on nsILoadContext, and then make sure that in the e10s necko setup the nsILoadContext is set up correctly.

(In reply to Boris Zbarsky [:bz] from comment #2) > The tight thing to do is presumably to add whatever identifying info is > needed on nsILoadContext, and then make sure that in the e10s necko setup > the nsILoadContext is set up correctly. Thanks for the quick reply! ochameau on IRC pointed me to loadContext.appId. [1] For Firefox OS can we use loadContext.appId? Is there a 1-to-1 mapping between apps and content processes? Can one app be started multiple times? (multiple instances within the same process or different processes?) Can one app have multiple content processes? Or can different but somehow related apps share a process? How about the system apps and the certified apps? If appId is usable in FxOS for our purposes, we will need a different way to identify content processes in the e10s-world of Firefox for desktop. Is this correct? [1] https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl#90

I don't know the answers to your questions; someone who's been active in b2g might....

Thanks bz! Vivien: please see comment 3 for the specific questions, and the related discussion. Can you please shed some light on how we can get the network monitor to work? Thank you!

I CCed some folks who may have answers for you.

(In reply to Mihai Sucan [:msucan] from comment #3) > (In reply to Boris Zbarsky [:bz] from comment #2) > > The tight thing to do is presumably to add whatever identifying info is > > needed on nsILoadContext, and then make sure that in the e10s necko setup > > the nsILoadContext is set up correctly. > > Thanks for the quick reply! > > ochameau on IRC pointed me to loadContext.appId. [1] > > For Firefox OS can we use loadContext.appId? Is there a 1-to-1 mapping > between apps and content processes? Can one app be started multiple times? IIRC appId has been written specifically for Firefox OS so yes. An application is opened only in one content process, and every new <iframe mozbrowser mozapp> will open into this process. In the current stage the system app, the browser app and the keyboard share the same process but this won't be true in a near future and all apps should lived into a separate process. Now the issue with appId will be if you want to activate the network monitor for content processes that does not have an appId associated with their loadContext. I believe that all iframes in the browser apps does not, and this is likely the same things for bookmarks opened from the homescreen (some calls them e.me bookmark but you can also add a bookmark to the homescreen from the web browser). > (multiple instances within the same process or different processes?) Can one > app have multiple content processes? Or can different but somehow related > apps share a process? How about the system apps and the certified apps? > > If appId is usable in FxOS for our purposes, we will need a different way to > identify content processes in the e10s-world of Firefox for desktop. Is this > correct? Sadly appId will tells you which apps has trigger the network connection but as I tried to said to Paul a few times, we support multiple <iframe mozbrowser mozapp> for the same app. It ends up that you can have multiple windows that are opened on the same process. And if you want to identify which one correctly appId is not enough. And since appId is reliable only for apps but is not reliable for web sites opened into a separate content process I believe something else needs to be found. Have you tried to look at loadContext.topFrameElement and see what is returned? This is all I know, I hope some of the networking folks CC'ed by Ehsan may help a little bit more.

Thank you Vivien for your reply. (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7) > (In reply to Mihai Sucan [:msucan] from comment #3) > > (In reply to Boris Zbarsky [:bz] from comment #2) > > > The tight thing to do is presumably to add whatever identifying info is > > > needed on nsILoadContext, and then make sure that in the e10s necko setup > > > the nsILoadContext is set up correctly. > > > > Thanks for the quick reply! > > > > ochameau on IRC pointed me to loadContext.appId. [1] > > > > For Firefox OS can we use loadContext.appId? Is there a 1-to-1 mapping > > between apps and content processes? Can one app be started multiple times? > > IIRC appId has been written specifically for Firefox OS so yes. An > application is opened only in one content process, and every new <iframe > mozbrowser mozapp> will open into this process. > > In the current stage the system app, the browser app and the keyboard share > the same process but this won't be true in a near future and all apps should > lived into a separate process. > > Now the issue with appId will be if you want to activate the network monitor > for content processes that does not have an appId associated with their > loadContext. I believe that all iframes in the browser apps does not, and > this is likely the same things for bookmarks opened from the homescreen > (some calls them e.me bookmark but you can also add a bookmark to the > homescreen from the web browser). > > > (multiple instances within the same process or different processes?) Can one > > app have multiple content processes? Or can different but somehow related > > apps share a process? How about the system apps and the certified apps? > > > > If appId is usable in FxOS for our purposes, we will need a different way to > > identify content processes in the e10s-world of Firefox for desktop. Is this > > correct? > > Sadly appId will tells you which apps has trigger the network connection but > as I tried to said to Paul a few times, we support multiple <iframe > mozbrowser mozapp> for the same app. It ends up that you can have multiple > windows that are opened on the same process. And if you want to identify > which one correctly appId is not enough. > > And since appId is reliable only for apps but is not reliable for web sites > opened into a separate content process I believe something else needs to be > found. Have you tried to look at loadContext.topFrameElement and see what is > returned? Paul, when the user connects to a Firefox OS device he picks the app to debug - is this correct? If yes, then we shouldn't be bothered if we get all of the network requests of that app from any of its multiple mozbrowser mozapp iframes. Is this correct? Or do we want to allow the user to pick only a specific mozbrowser mozapp iframe? Beyond that, I believe the main problem lies in the fact that appIds are not reliable for web sites opened in separate content processes, and when loadContext has no appId set (eg. system apps). > This is all I know, I hope some of the networking folks CC'ed by Ehsan may > help a little bit more. Can we add a processId property to nsILoadContext? Can scripts from content processes check their own PID?

Did you mean to needinfo? me? :-)

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9) > Did you mean to needinfo? me? :-) Yes. :) I wasn't sure who's appropriate to ask the last two questions. Sorry, I should've prefixed that paragraph with Ehsan, as I did for Paul.

I think Jason or Patrick would be the right people to talk to.

Patrick, can you please answer the following questions? > Can we add a processId property to nsILoadContext? Can scripts from content > processes check their own PID? Also see comment 7 and the rest of the comments for context. Thank you!

(In reply to Mihai Sucan [:msucan] from comment #12) > Patrick, can you please answer the following questions? > > > Can we add a processId property to nsILoadContext? Can scripts from content > > processes check their own PID? > > Also see comment 7 and the rest of the comments for context. > > Thank you! I don't know the answers to your questions. perhaps jdm or sicking.

No problem Patrick. Jonas, can you please answer the following questions? > Can we add a processId property to nsILoadContext? Can scripts from content > processes check their own PID? Also see comment 7 and the rest of the comments for context. Thank you!

Alex: can you please test this on a firefox OS device? Preferably with nightly codebase. This patch adds the appId to the ContentAppActor and changes the WebConsoleActor to use it for filtering network requests by appId. I expect this to fail, somehow, because some of the network logging stuff is expected to not work in content processes. I added some debug logging. Please copy/paste all logs from the start of connection, until the end, and attach them to the bug, so I can see when things fail. If things continue to fail badly, the next step is to start the network monitor in the main process, and filter network requests by appId there. Thank you!

Here is the log I got: I/Gecko ( 773): console.log: WebConsoleActor constructor {_prefix:..,_transport:..,_nextID:..,_actorPool:..,_extraPools:..,_actorResponses:..,_forwardingPrefixes:..,currentPacket:.., } ContentAppActor {conn:..,_browser:..,_tabbrowser:..,_tabActorPool:..,_extraActors:..,_onWindowCreated:..,appId:..,actorID:..,registeredPool:..,_tabActorPool2:..,_tabPool:..,_contextPool:..,threadActor:..,_attached:.., } parent actor ID conn0.child1030:tab1 appId 1030 I/GeckoDump( 773): WebConsoleActor constructor [object Object] [object Object] parent actor ID conn0.child1030:tab1 appId 1030 I/Gecko ( 773): console.log: WCA_onStartListeners window null appId 1030 I/GeckoDump( 773): WCA_onStartListeners window null appId 1030 I/Gecko ( 773): console.log: NetworkMonitor constructor window null appId 1030 I/GeckoDump( 773): NetworkMonitor constructor window null appId 1030 I/Gecko ( 773): console.log: WCA_onStartListeners window null appId 1030 I/GeckoDump( 773): WCA_onStartListeners window null appId 1030 The network monitor stays empty. Note that I only applied that to the device and patched that line in firefox: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/main.js#226

Thanks for testing. Looks like the NetworkMonitor in the child process doesn't get any notifications at al. Will submit a patch that starts the network monitor in the main process.

Sorry to bother you again. Updated patch to start the network monitor in the main process. This *should* work. I hope the patch doesnt have silly typos / errors. Like last time, please copy/paste the log. Thank you very much!

Comment on attachment 8360020 [details] [diff] [review] netlogging in the main process Review of attachment 8360020 [details] [diff] [review]: ----------------------------------------------------------------- With some additional fixes, I get big part of this code working, but still not enough to make the network panel work. I stopped debugging within utils.js _httpResponseExaminer. It seems to now be executed up to the end. So we either have silent exception, or something disable/non-working/missing. Here is the log (with the fixes mentioned) I/Gecko ( 1199): console.log: WebConsoleActor constructor parent actor ID conn0.child1025:tab1 appId 1025 I/Gecko ( 1199): console.log: WCA_onStartListeners window [object XrayWrapper [object Window]] appId 1025 listeners Array ["NetworkActivity"] I/Gecko ( 1199): NetworkMonitorChild constructor 1025 I/Gecko ( 1199): NetworkMonitorChild init() 1025 I/GeckoDump( 1042): ***** Got debug:netmonitor start 1025 I/Gecko ( 1042): console.log: NetworkMonitor constructor window null appId 1025 I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer start E/GeckoConsole( 1229): Could not read chrome manifest 'file:///system/b2g/chrome.manifest'. I/Gecko ( 1229): ###################################### forms.js loaded I/Gecko ( 1229): ############################### browserElementPanning.js loaded I/Gecko ( 1229): ######################## BrowserElementChildPreload.js loaded I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer getAppIdForRequest() result 1025 I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer getAppIdForRequest() result 1025 I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer getAppIdForRequest() result 1025 I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer getAppIdForRequest() result 1025 I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer getAppIdForRequest() result 1025 I/Gecko ( 1042): console.log: NM__httpResponseExaminer start I/Gecko ( 1042): console.log: NM__httpResponseExaminer getAppIdForRequest() result 1025 ::: toolkit/devtools/server/actors/webconsole.js @@ +488,5 @@ > case "NetworkActivity": > if (!this.networkMonitor) { > + if (appId) { > + this.networkMonitor = > + new DebuggerServer.NetworkMonitorChild(window, appId, this); NetworkMonitorChild doesn't have window first parameter: new DebuggerServer.NetworkMonitorChild(appId, this); ::: toolkit/devtools/webconsole/utils.js @@ +2014,5 @@ > + if (this.appId) { > + // Try to get the source appId of the request. > + let appId = NetworkHelper.getAppIdForRequest(channel); > + console.log("NM__httpResponseExaminer getAppIdForRequest() result", appId); > + if (!appId || appId !== this.appId) { I haven't really verified but we were getting into this condition. I imagine appId is a number and this.appId a string. It works with `appId != this.appId` @@ +2168,5 @@ > + if (this.appId) { > + // Try to get the source appId of the request. > + let appId = NetworkHelper.getAppIdForRequest(channel); > + console.log("NM__onRequestHeader getAppIdForRequest() result", appId); > + if (!appId || appId !== this.appId) { Same thing here.

(In reply to Alexandre Poirot (:ochameau) from comment #19) > Comment on attachment 8360020 [details] [diff] [review] > netlogging in the main process > > Review of attachment 8360020 [details] [diff] [review]: > ----------------------------------------------------------------- > > With some additional fixes, I get big part of this code working, but still > not enough to make the network panel work. Thank you for testing again and for the fixes. > I stopped debugging within utils.js _httpResponseExaminer. It seems to now > be executed up to the end. > So we either have silent exception, or something disable/non-working/missing. > Here is the log (with the fixes mentioned) > I/Gecko ( 1199): console.log: WebConsoleActor constructor parent actor ID > conn0.child1025:tab1 appId 1025 > I/Gecko ( 1199): console.log: WCA_onStartListeners window [object > XrayWrapper [object Window]] appId 1025 listeners Array ["NetworkActivity"] > I/Gecko ( 1199): NetworkMonitorChild constructor 1025 > I/Gecko ( 1199): NetworkMonitorChild init() 1025 > I/GeckoDump( 1042): ***** Got debug:netmonitor start 1025 > I/Gecko ( 1042): console.log: NetworkMonitor constructor window null appId > 1025 > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > E/GeckoConsole( 1229): Could not read chrome manifest > 'file:///system/b2g/chrome.manifest'. > I/Gecko ( 1229): ###################################### forms.js loaded > I/Gecko ( 1229): ############################### browserElementPanning.js > loaded > I/Gecko ( 1229): ######################## BrowserElementChildPreload.js > loaded > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer > getAppIdForRequest() result 1025 > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer > getAppIdForRequest() result 1025 > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer > getAppIdForRequest() result 1025 > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer > getAppIdForRequest() result 1025 > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer > getAppIdForRequest() result 1025 > I/Gecko ( 1042): console.log: NM__httpResponseExaminer start > I/Gecko ( 1042): console.log: NM__httpResponseExaminer > getAppIdForRequest() result 1025 This is promising, but I don't see anything from the activity distributor. I need to add more logging. > ::: toolkit/devtools/server/actors/webconsole.js > @@ +488,5 @@ > > case "NetworkActivity": > > if (!this.networkMonitor) { > > + if (appId) { > > + this.networkMonitor = > > + new DebuggerServer.NetworkMonitorChild(window, appId, this); > > NetworkMonitorChild doesn't have window first parameter: > new DebuggerServer.NetworkMonitorChild(appId, this); Meh, I was worried I'll forget something. > ::: toolkit/devtools/webconsole/utils.js > @@ +2014,5 @@ > > + if (this.appId) { > > + // Try to get the source appId of the request. > > + let appId = NetworkHelper.getAppIdForRequest(channel); > > + console.log("NM__httpResponseExaminer getAppIdForRequest() result", appId); > > + if (!appId || appId !== this.appId) { > > I haven't really verified but we were getting into this condition. > I imagine appId is a number and this.appId a string. > It works with `appId != this.appId` This is surprising. We should always get the same appId (including the type). Going to submit an updated patch with these two fixes and more logging, so we can, hopefully, determine what fails.

Included the fixes you suggested, added more logging, and wrapped a few methods in makeInfallible() calls, which should give us exceptions that happen during network monitoring.

Previous patch has an error wrt. imports. Fixed.

Comment on attachment 8360597 [details] [diff] [review] more debug logging (fixed imports) Review of attachment 8360597 [details] [diff] [review]: ----------------------------------------------------------------- After fixing a typo in utils.js, I got some stuff displayed \o/ For some reason my keyboard is currently broken, so that I can't write my wifi password and have a working connection... But it looks like there is an exception that prevent any panel to work (headers, cookies, ...) I only see a list of requests with method, file and domain columns being filled. Here are the logs: http://pastebin.mozilla.org/4081442 ::: toolkit/devtools/webconsole/utils.js @@ +2189,5 @@ > + console.log("NM__onRequestHeader 2"); > + > + if (this.appId) { > + // Try to get the source appId of the request. > + let appId = NetworkHelper.getAppIdForRequest(channel); channel -> aChannel

Alex, thanks a lot for the testing. That log looks very good - knowing the usual flow of those events. I'm glad no exceptions occurred in those places. The only problem is with the NetworkEventActorProxy which seems to throw pretty badly - and that would explain with you didnt see the headers/cookies/etc. I got my geeksphone today and I will do the testing myself (yay), if all goes well.

This patch is working for me with the latest Geeksphone nightly builds available for Peak. Things that work: network logging for apps - I can see request/response information and bodies. Concerns: - response bodies that are gzipped show compressed binary data in the netmonitor response body view. It seems this is different from what we expect on desktops - on desktops the network layer gives us the response body uncompressed. I suggest we file a follow up bug for this issue. We can decompress the body in the client. - the "edit and send" network request option doesn't work properly. See WCA_onSendHTTPRequest() from actors/webconsole.js. We use the tab's window XMLHttpRequest object to make requests because they get their requests associated to the content window (the network logger skips everything that is not associated to the window we want). In the content process the window.XMLHttpRequest belongs to the app, which seems to have some restrictions. Network requests only happen with the OPTIONS method, and custom headers are added as Access-Control-foo. I have some ideas how we could fix request replay, but I believe this is also a good thing to fix in a follow-up. I tried some simple workarounds, no luck. Please let me know if you find other problems as well. Thank you! Another thing: do we want this in b2g 1.4, 1.3 or other versions? I did the majority of this patch with b2g 1.3, then I moved to the b2g nightly. The patch doesn't apply cleanly on b2g 1.3, but it's easy to backport (no features missing, it's just rebasing). I believe this is useful enough for backporting.

I would imagine it may be too late for 1.3 as they were supposed to wrap up 1.3 developments to switch to 1.4 this week. But it looks like it has been delayed for last minute requests... Let's first try to land it to master!

Comment on attachment 8368108 [details] [diff] [review] bug917227-4.diff Review of attachment 8368108 [details] [diff] [review]: ----------------------------------------------------------------- I've done a high level review. We need to find a way to make it work without the certified apps priviledges, while ensuring we do not open unexpected priviledges! Also, about followups, that would be really great if we can somehow disable non-working features dynamically by havign some feature detection (communicating with the actor, checking version, or something). Otherwise, the patch works great, everything your described to be working was working as expected! I was able to see requests for the marketplace (hosted app) as well as e.me requests from the homescreen (certified app). ::: browser/devtools/main.js @@ -222,5 @@ > tooltip: l10n("netmonitor.tooltip", netMonitorStrings), > inMenu: true, > > isTargetSupported: function(target) { > - return !target.isApp; It isn't that simple, we should show the network panel on devices that will ship this fix, but not on the older ones. Do you think we can check for network actor availability or something? Also, that would be really handy to land that sole modification, to allow testing the device patch without having to patch firefox desktop. ::: toolkit/devtools/server/actors/webapps.js @@ +805,5 @@ > + // Pipe network monitor data from parent to child via the message manager. > + switch (action) { > + case "start": { > + debug("establishing network monitoring for app " + appId); > + let netMonitor = new NetworkMonitor(null, appId, this); This throws on production device, as devtools.debugger.forbid-certified-apps is set to true. That prevents webconsole actor to be loaded in parent process, so that NetworkMonitor, here, is undefined :/ I do not have a good picture of the whole code you are depending on in parent process, but it looks like, this will make it harder to load the necessary code in parent, while ensuring that we do not open ways to execute code in parent process. devtools.debugger.forbid-certified-apps was introduced to limit debugger server features to non-certified apps, so that you can't stole data, nor root the phone via the devtools. Ideally, in parent process, we will only load NetworkMonitor, without other dependencies, and ensure by all means that we can't hack it to reach parent process requests. @@ +837,5 @@ > + } > + } > + }.bind(this)); > + mm.addMessageListener("debug:netmonitor", onNetMonitorMessage); > + That makes me sad to add some actor specific code in the webapps actor. (it also applies to NetworkMonitorChild in child.js) We should at the least move this code in some network-specific file. Also, it may be easier and less tied to the implementation of child processes, to use the debugger protocol instead of the message manager to communicate between NetworkMonitor() and NetworkMonitorChild(). Finally, I have no idea if that's even possible, but do we really have to pipe the data back to the child? Can't we just send messages on the debugger server channel by using the tab actor id generated in the child? Something vaguely looking like that: # child.js addMessageListener("debug:connect", funtion () { sendAsyncMessage("debug:actor", { actor: ..., appId: ..., actors: { networkActorId: somehowComputeItsID() } }); }); # webapps.js mm.addMessageListener("debug:actor", function (msg) { let { actor, appId, actors } = msg.json NetworkMonitor(this.conn,actors.networkActorId); }); # network-monitor.js function NetworkMonitor(server,actorId) {} NetworkMonitor.prototype.doSomethingSomewhere= function () { this.server.send({from: this.actorId, ... }); }

> We need to find a way to make it work without the certified apps priviledges, If we need to enable certified apps debug, it's pretty :( Alex mentioned there might be a way to enable network notifications in the child process. Alex, how can we re-enable network notification in the child process? Maybe we can give it a try.

I think I already mentioned bug 806753, where various http related notifications have been disabled in the child, just because of the http objects being dispatched in these events can be "misleadingly" modified whereas any modification being made will have no effect. So I was wondering if we can somehow revive these events, so that network actor code starts receiving http notification in child process. The only thing that wouldn't work would be the request modification. I'm not sure that's a really important feature for on device development.

Did I say "pretty"? I meant "sad". Typo.

(In reply to Alexandre Poirot (:ochameau) from comment #26) > I would imagine it may be too late for 1.3 as they were supposed to wrap up > 1.3 developments to switch to 1.4 this week. But it looks like it has been > delayed for last minute requests... > > Let's first try to land it to master! Agreed. I was interested to know if 1.3.x releases can include 'important' patches, but first, let's see how this one turns out. Thanks for your review! (In reply to Alexandre Poirot (:ochameau) from comment #27) > Comment on attachment 8368108 [details] [diff] [review] > bug917227-4.diff > > Review of attachment 8368108 [details] [diff] [review]: > ----------------------------------------------------------------- > > I've done a high level review. > We need to find a way to make it work without the certified apps priviledges, > while ensuring we do not open unexpected priviledges! True. I forgot about this limitation and I was not aware that the console actor is explicitly disabled when debugging of certified apps is not allowed. Nonetheless, I am rather optimistic. See below. > Also, about followups, that would be really great if we can somehow disable > non-working features > dynamically by havign some feature detection (communicating with the actor, > checking version, or something). We need to determine what we want to disable and when. For one, we should disable 'network request replay', until we have a fix. > Otherwise, the patch works great, everything your described to be working > was working as expected! > I was able to see requests for the marketplace (hosted app) as well as e.me > requests from the homescreen (certified app). > > ::: browser/devtools/main.js > @@ -222,5 @@ > > tooltip: l10n("netmonitor.tooltip", netMonitorStrings), > > inMenu: true, > > > > isTargetSupported: function(target) { > > - return !target.isApp; > > It isn't that simple, we should show the network panel on devices that will > ship this fix, but not on the older ones. Do you think we can check for > network actor availability or something? Afaik we have a mechanism for the server to tell the client which features are supported. I will look into this. > Also, that would be really handy to land that sole modification, to allow > testing the device patch without having to patch firefox desktop. I'm not sure if I understand the request here. If you want, we can split the patch in two, no problem. One for the client, and one for the server stuff - would this be what you want? > ::: toolkit/devtools/server/actors/webapps.js > @@ +805,5 @@ > > + // Pipe network monitor data from parent to child via the message manager. > > + switch (action) { > > + case "start": { > > + debug("establishing network monitoring for app " + appId); > > + let netMonitor = new NetworkMonitor(null, appId, this); > > This throws on production device, as devtools.debugger.forbid-certified-apps > is set to true. That prevents webconsole actor to be loaded in parent > process, so that NetworkMonitor, here, is undefined :/ > I do not have a good picture of the whole code you are depending on in > parent process, but it looks like, this will make it harder to load the > necessary code in parent, while ensuring that we do not open ways to execute > code in parent process. > devtools.debugger.forbid-certified-apps was introduced to limit debugger > server features to non-certified apps, so that you can't stole data, nor > root the phone via the devtools. > > Ideally, in parent process, we will only load NetworkMonitor, without other > dependencies, and ensure by all means that we can't hack it to reach parent > process requests. No problem. I wasn't aware of this particular effect of the forbid-certifie-apps restriction. I can require() only the NetworkMonitor. These pieces of code do not depend on the console actor running in the main process. > @@ +837,5 @@ > > + } > > + } > > + }.bind(this)); > > + mm.addMessageListener("debug:netmonitor", onNetMonitorMessage); > > + > > That makes me sad to add some actor specific code in the webapps actor. (it > also applies to NetworkMonitorChild in child.js) > We should at the least move this code in some network-specific file. Good point. This is a good time to move NetworkMonitor code out of utils.js, and to put it into a new network-monitor.js, where I can add the new NetworkMonitorChild and NetworkEventActorProxy. > Also, it may be easier and less tied to the implementation of child > processes, to use the debugger protocol instead of the message manager to > communicate between NetworkMonitor() and NetworkMonitorChild(). To do this, we would need a new actor in the child server, or additional requests in the console actor. Technically, I favor this approach - the message manager. Are there any performance concerns, or some other reason why we should use the debugger protocol? > Finally, I have no idea if that's even possible, but do we really have to > pipe the data back to the child? Can't we just send messages on the > debugger server channel by using the tab actor id generated in the child? This is what I also wanted when I started working on the patch, but it quickly becomes 'complicated' and very 'hackish'. You need to have the child console actor able to give the client new instances of the NetworkEventActor which has its own notifications and request types. So, the client can wait for NetworkEventActor updates from the child, and it can send messages to get packets from the actor. Masquerading/faking the child NEA is easy in the main process - it's easy to send notifications as if they are sent by the child NEA. However, things become hairy/ugly when we need to intercept requests to the child process from the client, such that the NEA from the main process can reply. Also, things become ugly when you think of what actor ID can you assign to the NEA from the main process on behalf of the child process? How do you deal with requests to release the actor? (requests are sent to the child) You need some kind of special casing. > Something vaguely looking like that: > # child.js > addMessageListener("debug:connect", funtion () { > sendAsyncMessage("debug:actor", { > actor: ..., > appId: ..., > actors: { > networkActorId: somehowComputeItsID() > } > }); > }); > > # webapps.js > mm.addMessageListener("debug:actor", function (msg) { > let { actor, appId, actors } = msg.json > NetworkMonitor(this.conn,actors.networkActorId); > }); > > # network-monitor.js > function NetworkMonitor(server,actorId) {} > NetworkMonitor.prototype.doSomethingSomewhere= function () { > this.server.send({from: this.actorId, ... }); > } The NetworkMonitor doesn't have any actor ID of its own, only the console. The console actor gives clients new NetworkEventActors for every new request.

(In reply to Mihai Sucan [:msucan] from comment #31) > > Also, that would be really handy to land that sole modification, to allow > > testing the device patch without having to patch firefox desktop. > > I'm not sure if I understand the request here. If you want, we can split the > patch in two, no problem. One for the client, and one for the server stuff - > would this be what you want? Yes, and if we can land the client patch right away, that would be handy. > > Ideally, in parent process, we will only load NetworkMonitor, without other > > dependencies, and ensure by all means that we can't hack it to reach parent > > process requests. > > No problem. I wasn't aware of this particular effect of the > forbid-certifie-apps restriction. I can require() only the NetworkMonitor. > These pieces of code do not depend on the console actor running in the main > process. Note that bug 962577 is going to factor out connectToApp out of webapps actor, you will need to rebase once it lands. > > Also, it may be easier and less tied to the implementation of child > > processes, to use the debugger protocol instead of the message manager to > > communicate between NetworkMonitor() and NetworkMonitorChild(). > > To do this, we would need a new actor in the child server, or additional > requests in the console actor. Technically, I favor this approach - the > message manager. Are there any performance concerns, or some other reason > why we should use the debugger protocol? That scared me to see network monitor stuff in middle of webapps actor, but now that we factor connectToApp out of the actor, adding some tab actors-specific code around the message manager makes sense. Ideally, this code will be shared between app processes as well as firefox OOP tabs and may be also workers. > > Finally, I have no idea if that's even possible, but do we really have to > > pipe the data back to the child? Can't we just send messages on the > > debugger server channel by using the tab actor id generated in the child? > > This is what I also wanted when I started working on the patch, but it > quickly becomes 'complicated' and very 'hackish'. I was expecting such response ;) I wanted to ensure we considered such option. Have you also looked at the opposite option I mentioned in comment 29?

This patch includes only the browser/ changes. Mainly, this patch makes sure that the network monitor is enabled if the remote target supports the needed features. Options like network request replay and the new network performance stats are also disabled if they are not supported. I added traits for 'networkMonitor', 'customNetworkRequest' and 'networkPerformanceStatistics'. I am not sure of the last one, because the implementation uses tab actor reconfigure request. Should I remove the 'networkPerformanceStatistics' trait? We can rely on applicationType == 'browser', for now. The new net perf stats feature doesn't work with b2g, and it's not yet clear to me how it should work. The client is attached to the app, not to a web page tab, where it doesn't make sense to perform a reload with cache enabled, and a different reload with cache disabled, to show charts. I disabled this feature, for now, when connected to b2g. Should we file a bug about adding support for this feature in b2g? I believe it will only make sense once we allow the inspection/attachment to deeper levels of windows. For example, in the future we could allow attaching the toolbox to a tab in the Firefox browser running in b2g - or any other similar app.

This is the patch with the toolkit/ changes needed to get the network monitor working with b2g. Changes: - moved the network monitor code from webconsole/utils.js into a new network-monitor.js script. - moved the new NetworkMonitorChild (from child.js) into the new network-monitor.js file. Also moved NetworkEventActorProxy away from webapps.js. - network requests are now logged for uncertified apps, and you dont need to enable the debugging of certified apps. - added traits such that unsupported features in netmonitor are not shown. Concern: ConsoleProgressListener is used for tracking file:// loads and it's moot/useless with b2g. It would be very nice if we could show app:// file loads as well, especially for debugging purposes. Should we file a follow-up bug for getting CPL working with app files? Looking forward for your review. Thanks!

Not sure if it's still relevant, but re: comment 29, yes we could re-enable the http-on-* notifications in the child if you need them.

Comment on attachment 8375687 [details] [diff] [review] bug917227-browser-1.diff Review of attachment 8375687 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/main.js @@ +242,5 @@ > + let root = target.client.mainRoot; > + return root.traits.networkMonitor || > + // Compat with older browser releases which did not include the > + // trait, but they have a working network monitor. > + root.applicationType == "browser"; may be we should do root.traits.networkMonitor || !target.isApp, or, root.traits.networkMonitor || !target.isRemote to prevent breaking thunderbird and other apps.

(In reply to Alexandre Poirot (:ochameau) from comment #36) > Comment on attachment 8375687 [details] [diff] [review] > bug917227-browser-1.diff > > Review of attachment 8375687 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/main.js > @@ +242,5 @@ > > + let root = target.client.mainRoot; > > + return root.traits.networkMonitor || > > + // Compat with older browser releases which did not include the > > + // trait, but they have a working network monitor. > > + root.applicationType == "browser"; > > may be we should do root.traits.networkMonitor || !target.isApp, or, > root.traits.networkMonitor || !target.isRemote to prevent breaking > thunderbird and other apps. Also, for me target.client.mainRootwill was coming undefined for local connections. so you want target.isLocalTab before anything else.

s/target.client.mainRootwill/target.client.mainRoot

Comment on attachment 8375688 [details] [diff] [review] bug917227-toolkit-1.diff Review of attachment 8375688 [details] [diff] [review]: ----------------------------------------------------------------- Here is some comments, I'll take some time tomorrow to test it and get back to you. I wish this patch wasn't using appId, but a more general id that would also work for firefox e10s. But we can look at this in a followup. In my iframe selection patch, I was able to identify any frames across processes with a child id + window outer id within the child. But I'm pretty sure it isn't possible to fetch such info from network interfaces... ::: toolkit/devtools/server/actors/webapps.js @@ +818,5 @@ > + switch (action) { > + case "start": { > + debug("establishing network monitoring for app " + appId); > + let netMonitor = new NetworkMonitor(null, appId, this); > + netMonitor._messageManager = mm; Rather than adding _messageManager to the network monitor instance, and involving the app actor in this, I would spawn a dedicated callback to NetworkMonitor constructor with something like this: let onNetworkEvent = { onNetworkEvent: function (event, channel, netMonitor) { let proxy = new NetworkEventActorProxy(mm); mm.sendAsyncMessage("debug:netmonitor:newEvent", { id: proxy.id, event: event, }); return proxy; } }; let onNetMonitorMessage = function(msg) { ... let netMonitor = new NetworkMonitor(null, appId, onNetworkEvent); netMonitor.init(); ... }; @@ +843,5 @@ > + if (netMonitor) { > + netMonitor.destroy(); > + netMonitor._messageManager = null; > + this._netMonitorMap.delete(mm); > + } Can we have more than one network monitor per app? If not, we may get rid of the Map and just keep a local variable, and reject the call to "start" if we try to instanciate more than one monitor. @@ +933,5 @@ > + * data about the request is available. > + */ > + onNetworkEvent: function(event, channel, netMonitor) { > + let mm = netMonitor._messageManager; > + let proxy = new NetworkEventActorProxy(mm, this); It looks like NetworkEventActorProxy never uses `owner` (i.e. second argument, which is `this` here). ::: toolkit/devtools/server/actors/webbrowser.js @@ +65,5 @@ > + globalActorFactories: DebuggerServer.globalActorFactories, > + onShutdown: sendShutdownEvent > + }); > + actor.traits.customNetworkRequest = true; > + actor.traits.networkPerformanceStatistics = true; Instead of piling all sort of traits in the root actor, do you think we can add some actor-specific traits right into a given actor? Like here, could we add traits on the webconsole actor itself? ::: toolkit/devtools/server/actors/webconsole.js @@ +467,5 @@ > + if ("ContentAppActor" in DebuggerServer && > + this.parentActor instanceof DebuggerServer.ContentAppActor) { > + // Filter network requests by appId on Firefox OS devices. > + appId = this.parentActor.appId; > + messageManager = this.conn.transport.sender; Rather than hacking into the transport layer, wouldn't it be more explicit to store the message manager in the ContentAppActor? messageManager = this.parentActor.messageManager;

(In reply to Alexandre Poirot (:ochameau) from comment #36) > Comment on attachment 8375687 [details] [diff] [review] > bug917227-browser-1.diff > > Review of attachment 8375687 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/main.js > @@ +242,5 @@ > > + let root = target.client.mainRoot; > > + return root.traits.networkMonitor || > > + // Compat with older browser releases which did not include the > > + // trait, but they have a working network monitor. > > + root.applicationType == "browser"; > > may be we should do root.traits.networkMonitor || !target.isApp, or, > root.traits.networkMonitor || !target.isRemote to prevent breaking > thunderbird and other apps. Good idea. I will use root.traits.networkMonitor || !target.isApp. (In reply to Alexandre Poirot (:ochameau) from comment #39) > Comment on attachment 8375688 [details] [diff] [review] > bug917227-toolkit-1.diff > > Review of attachment 8375688 [details] [diff] [review]: > ----------------------------------------------------------------- > > Here is some comments, I'll take some time tomorrow to test it and get back > to you. Thanks! Looking forward to your reviews. > I wish this patch wasn't using appId, but a more general id that would also > work for firefox e10s. > But we can look at this in a followup. In my iframe selection patch, I was > able to identify any frames across processes with a child id + window outer > id within the child. > But I'm pretty sure it isn't possible to fetch such info from network > interfaces... Agreed. We discussed this and decided to use appId for now, until a new kind of ID is added to uniquely represent frames across processes. > ::: toolkit/devtools/server/actors/webapps.js > @@ +818,5 @@ > > + switch (action) { > > + case "start": { > > + debug("establishing network monitoring for app " + appId); > > + let netMonitor = new NetworkMonitor(null, appId, this); > > + netMonitor._messageManager = mm; > > Rather than adding _messageManager to the network monitor instance, and > involving the app actor in this, > I would spawn a dedicated callback to NetworkMonitor constructor with > something like this: > > let onNetworkEvent = { > onNetworkEvent: function (event, channel, netMonitor) { > let proxy = new NetworkEventActorProxy(mm); > mm.sendAsyncMessage("debug:netmonitor:newEvent", { > id: proxy.id, > event: event, > }); > return proxy; > } > }; > > let onNetMonitorMessage = function(msg) { > ... > let netMonitor = new NetworkMonitor(null, appId, onNetworkEvent); > netMonitor.init(); > ... > }; I will look into this. > @@ +843,5 @@ > > + if (netMonitor) { > > + netMonitor.destroy(); > > + netMonitor._messageManager = null; > > + this._netMonitorMap.delete(mm); > > + } > > Can we have more than one network monitor per app? > If not, we may get rid of the Map and just keep a local variable, > and reject the call to "start" if we try to instanciate more than one > monitor. Good idea. > @@ +933,5 @@ > > + * data about the request is available. > > + */ > > + onNetworkEvent: function(event, channel, netMonitor) { > > + let mm = netMonitor._messageManager; > > + let proxy = new NetworkEventActorProxy(mm, this); > > It looks like NetworkEventActorProxy never uses `owner` (i.e. second > argument, which is `this` here). Obsolete property, will remove. > ::: toolkit/devtools/server/actors/webbrowser.js > @@ +65,5 @@ > > + globalActorFactories: DebuggerServer.globalActorFactories, > > + onShutdown: sendShutdownEvent > > + }); > > + actor.traits.customNetworkRequest = true; > > + actor.traits.networkPerformanceStatistics = true; > > Instead of piling all sort of traits in the root actor, > do you think we can add some actor-specific traits right into a given actor? > Like here, could we add traits on the webconsole actor itself? I was also wondering about these traits. I should be able to put 'customNetworkRequestSupport' in the webconsole actor grip. Does that sound fine? The `networkPerformanceStatistics` feature is not handled by the console actor. The Network Monitor tools sends reload requests to the tabActor with the reconfigure request packet, enabling/disabling request caching. I will look into the details of this implementation. > ::: toolkit/devtools/server/actors/webconsole.js > @@ +467,5 @@ > > + if ("ContentAppActor" in DebuggerServer && > > + this.parentActor instanceof DebuggerServer.ContentAppActor) { > > + // Filter network requests by appId on Firefox OS devices. > > + appId = this.parentActor.appId; > > + messageManager = this.conn.transport.sender; > > Rather than hacking into the transport layer, wouldn't it be more explicit > to store the message manager in the ContentAppActor? > > messageManager = this.parentActor.messageManager; I wanted that and I wasn't sure if it's ok to put the message manager in the app actor. I will do. Thanks!

Updated the patch to address Alex's review comments: - using !target.isApp for fallback if !root.traits.networkMonitor. - moved the customNetworkRequest trait to the console actor. - removed the networkPerformanceStatistics trait. Replaced this trait with a 'reconfigure' trait on the tab actor.

Updated the patch to address Alex's review comments: - same updates as in the browser/ patch, plus more. - updated webapps actor to have even less glue code for the NetworkMonitor. Based on Alex's suggested I added a new NetworkMonitorManager which only needs a message manager instance to handle the rest. You can find NMM in network-monitor.js. - _messageManager is no longer shoe-horned into NetworkMonitor. - removed unused `owner` from NetworkEventActorProxy. - removed _netMonitorMap from the webapps actor. - added the `messageManager` property to the ContentAppActor. All console/debugger/netmon tests pass locally. Didn't get the chance to test with my b2g device, yet (small problem with the local carrier). I know of one potential bug, will test next week.

Comment on attachment 8383883 [details] [diff] [review] bug917227-toolkit-2.diff Review of attachment 8383883 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/webapps.js @@ +24,5 @@ > +}); > +XPCOMUtils.defineLazyGetter(this, "NetworkEventActorProxy", () => { > + return devtools.require("devtools/toolkit/webconsole/network-monitor") > + .NetworkEventActorProxy; > +}); You are only using NetworkMonitorManager now. ::: toolkit/devtools/server/actors/webconsole.js @@ +507,5 @@ > startedListeners.push(listener); > break; > case "NetworkActivity": > if (!this.networkMonitor) { > + if (appId && messageManager) { nit: may be we can move the appId, messageManager definition right here: if ("ContentAppActor" in DebuggerServer && this.parentActor instanceof DebuggerServer.ContentAppActor) { // Filter network requests by appId on Firefox OS devices. let appId = this.parentActor.appId; let messageManager = this.parentActor.messageManager; this.networkMonitor = new NetworkMonitorChild(appId, messageManager, this); } else { .. }

Comment on attachment 8383882 [details] [diff] [review] bug917227-browser-2.diff Review of attachment 8383882 [details] [diff] [review]: ----------------------------------------------------------------- Oops I meant r+ this patch.

Comment on attachment 8383883 [details] [diff] [review] bug917227-toolkit-2.diff Review of attachment 8383883 [details] [diff] [review]: ----------------------------------------------------------------- r- just for the exceptions, otherwise it seems to work fine and I don't have much to say about the code now. Thanks for all your efforts on this patch, we are getting close!! I'm seeing following exception, while debugging marketplace app. Also I tend to see each request 3 times, it may be related to the various exception I get. And sometimes I see the edit and resend button, we shouldn't see it, right? I/Gecko ( 1428): Handler function NM__httpResponseExaminer threw an exception: TypeError: this.getRequestLoadContext(...) is null I/Gecko ( 1428): Stack: NH_getAppIdForRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js:200:5 I/Gecko ( 1428): NM__httpResponseExaminer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:452:1 I/Gecko ( 1428): makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 I/Gecko ( 1428): Line: 200, column: 4 E/GeckoConsole( 1428): [JavaScript Error: "Handler function NM__httpResponseExaminer threw an exception: TypeError: this.getRequestLoadContext(...) is null E/GeckoConsole( 1428): Stack: NH_getAppIdForRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js:200:5 E/GeckoConsole( 1428): NM__httpResponseExaminer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:452:1 E/GeckoConsole( 1428): makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 E/GeckoConsole( 1428): Line: 200, column: 4" {file: "resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js" line: 56}] I/Gecko ( 1428): Handler function NM__httpResponseExaminer threw an exception: TypeError: this.getRequestLoadContext(...) is null I/Gecko ( 1428): Stack: NH_getAppIdForRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js:200:5 I/Gecko ( 1428): NM__httpResponseExaminer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:452:1 I/Gecko ( 1428): makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 I/Gecko ( 1428): Line: 200, column: 4 E/GeckoConsole( 1428): [JavaScript Error: "Handler function NM__httpResponseExaminer threw an exception: TypeError: this.getRequestLoadContext(...) is null E/GeckoConsole( 1428): Stack: NH_getAppIdForRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js:200:5 E/GeckoConsole( 1428): NM__httpResponseExaminer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:452:1 E/GeckoConsole( 1428): makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 E/GeckoConsole( 1428): Line: 200, column: 4" {file: "resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js" line: 56}] And this one when re-connecting to the marketplace: (open marketplace, open a toolbox, close the toolbox, reopen a toolbox, the marketplace app is reloaded and you get these exceptions) I/GeckoDump( 1613): Handler function DebuggerProgressListener.prototype.onStateChange threw an exception: TypeError: this._tabActor is null I/GeckoDump( 1613): Stack: DPL_onStateChange@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:1303:1 I/GeckoDump( 1613): makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 I/GeckoDump( 1613): BrowserTabActor.prototype.onReload/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:892:7 I/GeckoDump( 1613): makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 I/GeckoDump( 1613): Line: 1303, column: 0 E/GeckoConsole( 1613): [JavaScript Error: "Handler function DebuggerProgressListener.prototype.onStateChange threw an exception: TypeError: this._tabActor is null E/GeckoConsole( 1613): Stack: DPL_onStateChange@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:1303:1 E/GeckoConsole( 1613): makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:80:7 E/GeckoConsole( 1613): BrowserTabActor.prototype.onReload/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:892:7 ::: toolkit/devtools/webconsole/network-monitor.js @@ +1134,5 @@ > + * Message from the content. > + */ > + onNetMonitorMessage: DevToolsUtils.makeInfallible(function _onNetMonitorMessage(msg) { > + let { action, appId } = msg.json; > + debug("***** Got debug:netmonitor " + action + " " + appId); debug isn't defined in this scope and throws. @@ +1139,5 @@ > + > + // Pipe network monitor data from parent to child via the message manager. > + switch (action) { > + case "start": { > + debug("establishing network monitoring for app " + appId); Same thing here.

fyi, bug 962577 should land shortly and finally move connectToApp into main.js and be renamed to connectToChild. Then bug 937172 may also land shortly after that and may also conflict around connectToChild.

It's already been mentioned here, but loadContext.topFrameElement is the correct way to map network requests to their originating DOM element across processes. In desktop Firefox with e10s, loadContext.topFrameElement will be the <browser> element that initiated the request. In b2g, I think it will be the <iframe mozbrowser>. In both cases, you can get the element's message manager. Unfortunately, topFrameElement is not set for in-process loads. We could fix that if necessary, I think. It looks like you already have fallback paths in place using associatedWindow, so maybe that's not necessary. Anyway, I think topFrameElement is definitely the way to go.

(In reply to Alexandre Poirot (:ochameau) from comment #43) > Comment on attachment 8383883 [details] [diff] [review] > bug917227-toolkit-2.diff > > Review of attachment 8383883 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/devtools/server/actors/webapps.js > @@ +24,5 @@ > > +}); > > +XPCOMUtils.defineLazyGetter(this, "NetworkEventActorProxy", () => { > > + return devtools.require("devtools/toolkit/webconsole/network-monitor") > > + .NetworkEventActorProxy; > > +}); > > You are only using NetworkMonitorManager now. Fixed. > ::: toolkit/devtools/server/actors/webconsole.js > @@ +507,5 @@ > > startedListeners.push(listener); > > break; > > case "NetworkActivity": > > if (!this.networkMonitor) { > > + if (appId && messageManager) { > > nit: may be we can move the appId, messageManager definition right here: > if ("ContentAppActor" in DebuggerServer && > this.parentActor instanceof DebuggerServer.ContentAppActor) { > // Filter network requests by appId on Firefox OS devices. > let appId = this.parentActor.appId; > let messageManager = this.parentActor.messageManager; > this.networkMonitor = > new NetworkMonitorChild(appId, messageManager, this); > } else { > .. > } Not really. We start the net monitor only if the start listeners request asks for that, see the while-loop below. (In reply to Alexandre Poirot (:ochameau) from comment #45) > Comment on attachment 8383883 [details] [diff] [review] > bug917227-toolkit-2.diff > > Review of attachment 8383883 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- just for the exceptions, otherwise it seems to work fine and I don't have > much to say about the code now. > Thanks for all your efforts on this patch, we are getting close!! True that. > I'm seeing following exception, while debugging marketplace app. > Also I tend to see each request 3 times, it may be related to the various > exception I get. > And sometimes I see the edit and resend button, we shouldn't see it, right? Friday I didnt have time to test with the b2g device, hence the exceptions. I havent seen network requests being doubled-tripled, but I did play with debugging two apps at once and netmonitor doesn't correctly filter events/packets that arrive from the server, so you can see requests from all currently-debugging apps in all netmonitor toolbox instances. I have a fix for this in my upcoming patch (the browser/ patch). (the webconsole doesn't need the fix, it already correctly filters out packets from unknown actors.) > I/Gecko ( 1428): Handler function NM__httpResponseExaminer threw an > exception: TypeError: this.getRequestLoadContext(...) is null Right, nsILoadContext is not available for all kinds of requests we can catch. > And this one when re-connecting to the marketplace: > (open marketplace, open a toolbox, close the toolbox, reopen a toolbox, the > marketplace app is reloaded and you get these exceptions) > I/GeckoDump( 1613): Handler function > DebuggerProgressListener.prototype.onStateChange threw an exception: > TypeError: this._tabActor is null I haven't seen this error. I tried to reconnect, always WFM. > ::: toolkit/devtools/webconsole/network-monitor.js > @@ +1134,5 @@ > > + * Message from the content. > > + */ > > + onNetMonitorMessage: DevToolsUtils.makeInfallible(function _onNetMonitorMessage(msg) { > > + let { action, appId } = msg.json; > > + debug("***** Got debug:netmonitor " + action + " " + appId); > > debug isn't defined in this scope and throws. Fixed.

(In reply to Bill McCloskey (:billm) from comment #47) > It's already been mentioned here, but loadContext.topFrameElement is the > correct way to map network requests to their originating DOM element across > processes. In desktop Firefox with e10s, loadContext.topFrameElement will be > the <browser> element that initiated the request. In b2g, I think it will be > the <iframe mozbrowser>. In both cases, you can get the element's message > manager. This is good to know - wrt. e10s support. I played a bit with topFrameElement and it seems to work with b2g. I havent tested with e10s yet. I will attach a patch for Alex to try. Thanks!

Fix for netmonitor to not display network requests from actors it is not attached to. See netmonitor-controller.js and webconsole/client.js for small changes.

Fixes for exceptions and traits. Now you should not see the 'resend' and 'performance analysis' options in netmonitor when connected to b2g. All tests pass locally and I tested the patch with my b2g device.

Quick experiment to use topFrameElement instead of appId. It seems to work well with b2g. Should I finish this up? I think yes, since we need this for e10s. Should make it easy to get network logging working with the e10s patches from bug 937172.

Comment on attachment 8386359 [details] [diff] [review] bug917227-topFrameElement.diff I don't know how much work it would require, but yes, that would be really great! We would have the network monitor working everywhere in one step \o/

Updated to use topFrameElement for filtering network requests, along with appId.

The previous patch had some oranges in a try push. This patch fixes the errors. Green try push of both patches: https://tbpl.mozilla.org/?tree=Try&rev=022c8e3f9a20

Comment on attachment 8386846 [details] [diff] [review] bug917227-toolkit-4.diff Review of attachment 8386846 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for fixing all these nits I kept sending! Note that bug 962577 and bug 937172 just landed and will need some rebase. There is some disconnection issue most likely due to bug 966991, but it sounds like something we can fix in a followup as we already reached a pretty decent size for a single patch... ::: toolkit/devtools/server/actors/webbrowser.js @@ +716,5 @@ > type: "tabAttached", > threadActor: this.threadActor.actorID, > cacheEnabled: this._getCacheEnabled(), > + javascriptEnabled: this._getJavascriptEnabled(), > + traits: { reconfigure: true }, Similarely to what you have done for the web console actor, we could make traits be a property of BrowserTabActor, so that childtab.js can just modify its reconfigure attribute instead of overloading attach. function BrowserTabActor(...) { this.traits = {reconfigure: true}; } ///// function ContentAppActor(...) { BrowserTabActor.call(...); this.traits.reconfigure = false; }

(In reply to Alexandre Poirot (:ochameau) from comment #56) > Comment on attachment 8386846 [details] [diff] [review] > bug917227-toolkit-4.diff > > Review of attachment 8386846 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, thanks for fixing all these nits I kept sending! > Note that bug 962577 and bug 937172 just landed and will need some rebase. Rebase done. Things continue to work. > There is some disconnection issue most likely due to bug 966991, > but it sounds like something we can fix in a followup as we already reached > a pretty decent size for a single patch... Agreed. Done some testing with your patch from github [1] and I figured out some things. Will add details in my next comments. [1] https://github.com/ochameau/mozilla-central/commit/7aa54889bdd9e498264f5cc08f807e431e656e83 > ::: toolkit/devtools/server/actors/webbrowser.js > @@ +716,5 @@ > > type: "tabAttached", > > threadActor: this.threadActor.actorID, > > cacheEnabled: this._getCacheEnabled(), > > + javascriptEnabled: this._getJavascriptEnabled(), > > + traits: { reconfigure: true }, > > Similarely to what you have done for the web console actor, we could make > traits be a property of BrowserTabActor, so that childtab.js can just modify > its reconfigure attribute instead of overloading attach. > function BrowserTabActor(...) { > this.traits = {reconfigure: true}; > } > ... Good suggestion. Done. Will attach updated patch.

Only rebased this patch. Thanks for your reviews and patience.

This patch avoids duplicating network requests after subsequent reconnects to the same app for b2g. Based on local testing, this patch does not depend on bug 966991. Please confirm. I rebased your patch from github (thanks!) and I rebased it. See: https://pastebin.mozilla.org/4543702 Your patch improves the disconnect sequence but it's not enough for this particular case. See onDisconnect() from webapps.js, getAppActor(). The disconnect function is not invoked if you disconnect with the toolbox. onDisconnect() is only invoked by onMessageManagerDisconnect() from DebuggerServer.connectToChild(). onMessageManagerDisconnect() is called only when the observer service sends the message-manager-disconnect notification. This doesn't happen during a toolbox disconnect - I think it happens when the app dies for whatever reason. The onDisconnect() function from getAppActor() destroys the NetworkMonitorManager and the NetworkMonitor. Because this is not called, these listeners leak and they continue to monitor network activity after app manager disconnects. When you reconnect the debugger server continues to receive network logs from the child app, hence the duplicates. Based on logging I saw that WCA_disconnect() is still called, even without the patch for bug 966991, because the connection dies when the toolbox disconnects - the server seems clever enough. Which means the WebConsoleActor has a chance to call NetworkMonitorChild.destroy(). This patch changes NMC.destroy() to send a message via messagemanager to the parent process to ask for netmonitor destroy (not just stop). This makes the network monitor to properly cleanup itself, even if a part of the connection between the parent and child remains active. No more network request dupes after reconnect. As time allows, please review this patch so I can land these patches ASAP. I would like to avoid further bitrot and to get this in Firefox 30. Thanks! (in this comment I'm mentioning the source code of the latest fx-team, after-rebase-world)

Comment on attachment 8388796 [details] [diff] [review] bug917227-disconnect-1.diff Review of attachment 8388796 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I haven't been able to get duplicated entries if I disconnect, nor if I just close and reopen the toolbox.

Comment on attachment 8388778 [details] [diff] [review] bug917227-toolkit-5.diff Review of attachment 8388778 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/webapps.js @@ +841,5 @@ > map.delete(mm); > + if (netMonitor) { > + netMonitor.destroy(); > + netMonitor = null; > + } Note that, we should move all modifications made to webapps actor into main.js:connectToChild in order to enable the netmon on e10s firefox. That can be done in a e10s specific followup.

(In reply to Alexandre Poirot (:ochameau) from comment #61) > Comment on attachment 8388778 [details] [diff] [review] > bug917227-toolkit-5.diff > > Review of attachment 8388778 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/devtools/server/actors/webapps.js > @@ +841,5 @@ > > map.delete(mm); > > + if (netMonitor) { > > + netMonitor.destroy(); > > + netMonitor = null; > > + } > > Note that, we should move all modifications made to webapps actor into > main.js:connectToChild in order to enable the netmon on e10s firefox. > That can be done in a e10s specific followup. True. Other changes might be needed as well.

Landed: https://hg.mozilla.org/integration/fx-team/rev/731be5361993 https://hg.mozilla.org/integration/fx-team/rev/493f2ddd14ba https://hg.mozilla.org/integration/fx-team/rev/15b0a825f153 Thank you!

https://hg.mozilla.org/mozilla-central/rev/731be5361993 https://hg.mozilla.org/mozilla-central/rev/493f2ddd14ba https://hg.mozilla.org/mozilla-central/rev/15b0a825f153

Filed follow-up bugs: Bug 982319, Bug 982603 and Bug 982606, for things we can further improve.