769298 - Avoid logging script errors coming from private windows in the global error console

Status: RESOLVED FIXED

(Toolkit Graveyard :: Error Console, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Philip Chee]

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

Comment 2

[(no longer active)]

(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.

Comment 3

[Josh Matthews [:jdm]]

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.

Comment 4

[Andres Hernandez [:andreshm]]

I'll take a look at this one.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

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 6

[Josh Matthews [:jdm]]

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 7

[[e-mail]]

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.

Comment 8

[Josh Matthews [:jdm]]

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

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

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;
}

Comment 10

[[e-mail]]

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.

Comment 11

[(no longer active)]

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].

Comment 12

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

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

Comment 13

[(no longer active)]

(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...

Comment 14

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

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.

Comment 15

[Josh Matthews [:jdm]]

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

Comment 16

[Andres Hernandez [:andreshm]]

ok thanks!

Comment 17

[Josh Matthews [:jdm]]

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

Comment 18

[Josh Matthews [:jdm]]

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

Comment 19

[Andres Hernandez [:andreshm]]

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

Comment 20

[Josh Matthews [:jdm]]

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.

Comment 21

[Andres Hernandez [:andreshm]]

Attachment: 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();
> }

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.

Comment 22

[(no longer active)]

(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 ;-)

Comment 23

[Andres Hernandez [:andreshm]]

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

Comment 24

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d73a19586b4

Comment 25

[[e-mail]]

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.)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d73a19586b4

Should this have a test?

Comment 27

[(no longer active)]

(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.

Comment 28

[Ioana (away)]

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.