Closed Bug 1067576 Opened 5 years ago Closed 5 years ago

[e10s] Console.jsm doesn't work from child process on Windows

Categories

(DevTools :: General, defect)

All
Windows 7
defect
Not set

Tracking

(e10sm3+)

RESOLVED FIXED
Firefox 35
Tracking Status
e10s m3+ ---

People

(Reporter: Tomislav, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1060093 +++

Ally says the debugger and console output work for JS running in the e10s content process when testing with the "File > New e10s Window", but not with the "browser.tabs.remote.autostart" pref enabled. The "New e10s Window" is a temporary testing feature; autostart enables the e10s code we plan to ship, including extra add-on compatibility shims that apparently interfere with the devtools.

Ally recommends that we fix the debugger and console for our e10s "M2" milestone so add-on developers have a fighting chance of fixing their add-ons' e10s problems.
bug 1060093 was scoped down to just debugging JS in the child process, so this one is about the Console.jsm not working from the child process on windows:

> hey Mike, i tested console in the child process on windows, using
> scratchpad, and it doesn't seem to show up, either in the browser console
> nor the browser toolbox console:
> 
>   let mm = gBrowser.addTab('data:text/html,dumb').linkedBrowser.messageManager;
> 
>   mm.addMessageListener(7, function() {
>     console.log('truc'); 
>   })
> 
>   mm.loadFrameScript('data:,new ' + function() {
>     let CA = Components.utils.import("resource://gre/modules/devtools/Console.jsm", {});
>     let console = new CA.ConsoleAPI();
>     console.log('blah');    
>     sendAsyncMessage(7);
>   }, true);
> 
> according to my discussion with Panos and your testing (on OSX), i
> understand this should work, so maybe it's a windows-only bug?


and note that browser console is totally busted in current Nightly (unrelated bug), and the last one where this could be reproduced is from 9/13.
tracking-e10s: --- → ?
No longer depends on: 1060093
OS: All → Windows 7
Priority: P3 → --
Summary: [e10s] console doesn't work on windows in child process → [e10s] Console.jsm doesn't work from child process on Windows
Attached patch console-log (obsolete) — Splinter Review
This patch is a hack. It watches for console-api-log-message notifications in the child and forwards them to the parent. However, if the arguments to console.log are objects, it needs to replace them with the string "<unavailable>" since we can't send objects using the message manager.

The right fix for this is to run a webconsole actor in the child process. However, it seems like that would have many of the same complications as bug 1060093. Right now I'd just like to get something working.

If not being able to send objects is a big problem, then we could use CPOWs. However, I'd really like to keep those out of devtools.

Panos, does this seem okay to you? It would really be nice if add-on authors could use console.log.
Attachment #8489755 - Flags: feedback?(past)
Comment on attachment 8489755 [details] [diff] [review]
console-log

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

I'm guessing this isn't the right patch?
Attachment #8489755 - Flags: feedback?(past)
(In reply to Bill McCloskey (:billm) from comment #2)
> If not being able to send objects is a big problem, then we could use CPOWs.
> However, I'd really like to keep those out of devtools.

Logging objects to the console is quite useful to trace behavior or to inspect them in the object inspector thingy, just because something runs in a frame script doesn't change that.

Maybe sending it in a pre-serialized form to display it in the console output + a unique ID that can be used to fetch a CPOW on demand if the user clicks on it to inspect the object. That could avoid performance penalties while still maintain current functionality.
Attached patch console-log v2 (obsolete) — Splinter Review
Sorry. Here is the patch.
Attachment #8489755 - Attachment is obsolete: true
Attachment #8490037 - Flags: feedback?(past)
Comment on attachment 8490037 [details] [diff] [review]
console-log v2

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

So if I got it right, console.log works from content but not from chrome code in child processes, because the console actor in the child process drops the console-api-log-event in the chrome case (no innerWindowID), even though it receives it for both cases. Is that correct?

I don't think the approach in this patch is too bad, as far as hacks go. It will certainly provide add-on authors with basic debugging capabilities.
Attachment #8490037 - Flags: feedback?(past) → feedback+
Assignee: nobody → wmccloskey
Comment on attachment 8490037 [details] [diff] [review]
console-log v2

Yes, that's correct. I guess I'll switch this to review then.
Attachment #8490037 - Flags: feedback+ → review?(past)
Comment on attachment 8490037 [details] [diff] [review]
console-log v2

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

Can we add a test to make sure we don't accidentally regress this?
Attachment #8490037 - Flags: review?(past) → review+
Oh, and I'm not a toolkit/ peer (apart from toolkit/devtools), so you might want to get a formal approval from someone who is.
Comment on attachment 8490037 [details] [diff] [review]
console-log v2

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

I'm a toolkit peer and this doesn't look right to me. The browser-child framescript is going to get run for every tab in the child process, each one will receive and relay every console message. So you'll get a duplicate console message logged for every open tab.

You need some singleton in the child process to do the relaying instead, maybe add the listener in to BrowserUtils.jsm?
Attachment #8490037 - Flags: review+ → review-
Attached patch console-log v3Splinter Review
This patch adds a new service that runs in the parent process and another for the child process. It builds on top of bug 930243. This probably seems like overkill for this bug, but I think these things will be useful for other facilities.

I also moved the loading of browser-content.js earlier so that all content scripts can use console.*.
Attachment #8490037 - Attachment is obsolete: true
Attachment #8496309 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8496309 [details] [diff] [review]
console-log v3

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

Looks good but I'm not sure the name "global" is right. Global is already used to mean other things in our platform and this doesn't fit in so I think something else would make more sense. How about ProcessSingleton?
Attachment #8496309 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/c5e310d17e58
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Duplicate of this bug: 1079275
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.