The PB service currently clears the error console when leaving PB mode. For per-window PB, this won't work. Instead, we should avoid storing any content errors originating from private windows in the global error console, but ensure that they are propagated to per-window error consoles.

Are you talking about the toolkit error console or the Firefox web console?

(In reply to Philip Chee from comment #1) > Are you talking about the toolkit error console or the Firefox web console? The toolkit error console. The firefox web console is already per-window, so we don't need to worry about it.

Using code similar to http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/WebConsoleUtils.jsm?force=1#175, we'll want to check the nsIScriptError path in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/content/consoleBindings.xml#159 for a non-zero innerWindowID and return if http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PrivateBrowsingUtils.jsm#10 returns true for the window obtained.

I'll take a look at this one.

I tried to use the innerWindowId, however the nsIDOMWindowUtils have no getInnerWindowWithId method. See: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDOMWindowUtils. So, I used the outerWindowId with nsIDOMWindowUtils::getOuterWindowWithId.

Comment on attachment 655028 [details] [diff] [review] Patch v1 Thanks Andres! I'll let somebody who knows the toolkit error console code review this.

Comment on attachment 655028 [details] [diff] [review] Patch v1 I can see a problem with this scheme: if you close the private browsing window and then open the Error Console, you would see all the private errors, because we have no way of telling whether closed windows were private. >+ // filter private windows >+ if (scriptError.innerWindowID != 0) { Use outerWindowID for consistency? >+ let win = Services.wm.getMostRecentWindow(null); >+ if (win) { >+ let windowUtils = >+ win.QueryInterface(Ci.nsIInterfaceRequestor). >+ getInterface(Ci.nsIDOMWindowUtils); The Error Console is a window, so just use window.QueryInterface etc.

Hum. I guess we either need to keep an array of private window IDs somewhere or store privacy information on in nsIScriptError :/

I updated the patch with Neil suggestions. Also, it happens than when the Error Console is open and there are errors, the obtained scriptWindow is always null. So, all errors still appear, because it can't check if the scriptWindow is private or not. The scriptWindow is not null, only when Error Console is already opened when the error appears. let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor). getInterface(Ci.nsIDOMWindowUtils); let scriptWindow = windowUtils.getOuterWindowWithId(scriptError.outerWindowID); Looking at http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsScriptError.cpp#107 on the InitWithWindowID method, there is a window object, from where it gets the outerWindowId. We can try to use that window to check if it's private (not sure how) and set a new 'isFromPrivateWindow' attribute to nsScriptError, then we can just filter private windows on the Error Console by doing: if (scriptError.isFromPrivateWindow) { return; }

Brainstorming: A possible alternative might be to have a method on the console service that deletes error messages for a given inner window ID, called by private windows when they are destroyed.

I sort of lean towards the suggestion in comment 9. Basically, if we have the option of dealing with the privacy mode proactively as opposed to retroactively, it's always preferable. Andres, it seems like you have made great progress here. Do you want to continue with your patch here? Two notes: * Please use the new PrivateBrowsingUtils.isWindowPrivate API as opposed to accessing usePrivateBrowsing directly. * To see whether a window object is in private mode in C++, you need to get a docshell from nsPIDOMWindow using GetDocShell(), then do_QueryInterface the docshell to get an nsILoadContext* out of it, and then call UsePrivateBrowsing on it. One simple example of how to do that is in attachment 613368 [details] [diff] [review].

This patch adds an 'isFromPrivateWindow' attribute to the script error. Then, is easier to filter private windows on the error console.

(In reply to Andres Hernandez [:andreshm] from comment #12) > Created attachment 672412 [details] [diff] [review] > Patch v3 > > This patch adds an 'isFromPrivateWindow' attribute to the script error. > Then, is easier to filter private windows on the error console. So, you need to change the uuid of nsIScriptError for one thing. Also, did you test this patch with both the global and per-window PB builds? I think we might want to do the consoleBindings.xml part only in per-window builds...

I updated the uuid of nsIScriptError. > Also, did you test this patch with both the global and per-window PB builds? > I think we might want to do the consoleBindings.xml part only in per-window > builds... It works fine on both global and per-window PB builds. Please confirm if this should be just for per-window builds. I think we should keep it on both, since is the same idea.

Ehsan's correct; we should retain the existing behaviour while using the global service so as not to confuse web developers who still use the global console. I made that change, updated the iid, and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/12002e126b39

ok thanks!

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d439018c0523 for crashes.

Andres, could you fix up the patch to avoid the failures in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=12002e126b39?

Josh, I'm not sure what should I fix on the path. I just see some PROCESS-CRASH (application crashed) errors, but nothing related to the patch. I also pushed it to try for a second test and is all green, see https://tbpl.mozilla.org/?tree=Try&rev=07539b0a7343

With regards to the try push, you only tested mochitest-other, but the crashes occurred in mochitest-{2,3}, mochitest-browser-chrome, and crashtests. I suggest running the tests that failed locally; it looks like you should be able to reproduce the crashes, given how universal they were. The logs should show what test was running when the crash occurred.

According to the log, the issue is when loadContext was null. I tested locally on a failing test and this patch fixes it. > nsIDocShell* docShell = window->GetDocShell(); > nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(docShell); > if (loadContext) { > mIsFromPrivateWindow = loadContext->UsePrivateBrowsing(); > } I was going to run it on try with "try: -b do -p all -u crashtest,crashtest-ipc,mochitest-2,mochitest-3,mochitest-bc -t none", but looks like try is closed at the moment.

(In reply to Andres Hernandez [:andreshm] from comment #21) > Created attachment 673454 [details] [diff] [review] > Patch v5 > > According to the log, the issue is when loadContext was null. I tested > locally on a failing test and this patch fixes it. > > > nsIDocShell* docShell = window->GetDocShell(); > > nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(docShell); > > if (loadContext) { > > mIsFromPrivateWindow = loadContext->UsePrivateBrowsing(); > > } Makes sense. > I was going to run it on try with "try: -b do -p all -u > crashtest,crashtest-ipc,mochitest-2,mochitest-3,mochitest-bc -t none", but > looks like try is closed at the moment. Hmm, try seems to be open now that I checked. Can you please try again? (no pun intended ;-)

Try run finished: https://tbpl.mozilla.org/?tree=Try&rev=f824d1a46f2a

Sorry for failing to spot the missing UUID change. (I'm only here because I crashed in InitWithWindowID but I see that's being taken care of.)

https://hg.mozilla.org/mozilla-central/rev/0d73a19586b4 Should this have a test?

(In reply to comment #26) > Should this have a test? We already have a test for the global PB mode. We need to port that test to the per-window world, but that will happen in another bug.

This issue still reproduces on the current Aurora - 20130213042019 (checked on Windows 7 64bit and Mac OSX 10.8). I open the Error Console (Ctrl+Shift+J), then go to an already opened private window. When loading a new page or reloading a page, all the corresponding warnings are displayed in the console.