Bug 104383 – Web Inspector: Evaluate private browsing mode's effect on console messages.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mike West]

Some console messages are gated on not being in private browsing mode  (e.g. CachedResourceLoader::printAccessDeniedMessage).

1. Is this still relevant? Do console messages end up in system logs somewhere?
2. If it's relevant, how can we avoid making every call site do the check? Centralizing it somewhere (in Console?) might make sense.
3. What about the strange printf'ing we do in Console::addMessage? Is that necessary? If so, it should almost certainly be gated on private browsing.
4. More questions?

[Mike West]

Adam added this check in particular was added 4 years ago in http://trac.webkit.org/changeset/34720. :)

No bug number, however. I'll dig around for other occurrences.

[Mike West]

DOMWindow::printErrorMessage used to be in the JSC bindings as printErrorMessageForFrame, added in http://trac.webkit.org/changeset/49963  No commentary for that bit, however.

Monday morning, I'll dig a bit more into which messages these two methods are gating on privacy grounds.

[Mike West]

+jochen, who has opinions.

(In reply to comment #3)
> +jochen, who has opinions.

According to http://trac.webkit.org/browser/trunk/Source/WebCore/page/Settings.h#L161 we don't record any console message in private browsing mode.

I agree that this should probably be enforced in the Console

[Alexey Proskuryakov]

> 3. What about the strange printf'ing we do in Console::addMessage? Is that necessary?

It implements WebKit1 Mac SPI, +[WebCoreStatistics setShouldPrintExceptions:].

[Timothy Hatcher]

(In reply to comment #5)
> > 3. What about the strange printf'ing we do in Console::addMessage? Is that necessary?
> 
> It implements WebKit1 Mac SPI, +[WebCoreStatistics setShouldPrintExceptions:].

And it is something I plan to add to WK2. I have a local patch I use to make it work in WK2. It makes debugging the Web Inspector easier.

[Mike West]

(In reply to comment #4)
> (In reply to comment #3)
> > +jochen, who has opinions.
> 
> According to http://trac.webkit.org/browser/trunk/Source/WebCore/page/Settings.h#L161 we don't record any console message in private browsing mode.

Hrm. Ok.

I don't understand the privacy violation of console messages. Are they potentially logged to some persistant storage? As long as they're transient, what is the risk we're protecting users against?

[Timothy Hatcher]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > +jochen, who has opinions.
> > 
> > According to http://trac.webkit.org/browser/trunk/Source/WebCore/page/Settings.h#L161 we don't record any console message in private browsing mode.
> 
> Hrm. Ok.
> 
> I don't understand the privacy violation of console messages. Are they potentially logged to some persistant storage? As long as they're transient, what is the risk we're protecting users against?

They can go to the system log (seen via Console.app) if you just launch the app via double click (like a user). If you launch via the Terminal or Xcode it dumps to the TTY and not the system log.

[Mike West]

(In reply to comment #8)

> They can go to the system log (seen via Console.app) if you just launch the app via double click (like a user). If you launch via the Terminal or Xcode it dumps to the TTY and not the system log.

Thanks for the detail! Are you talking about the printf'd messages, or the messages sent to the console?

I see three things happening in Console::addMessage:

1. The message is sent to a ChromeClient.
2. The message is sent to InspectorInstrumentation.
3. The message is (potentially) printf'd.

Do all three potentially leak information to the system log? If possible, I'd like to retain #2, as I haven't seen any code in Inspector that would write information outside the context of the browsing session, but I agree that the other two should probably be gated on private browsing mode.

I'll put up a patch for discussion on Monday.

Can't you turn private browsing mode on and off during runtime? In that case you might want to clear the console messages stored in the inspector when private browsing is disabled again.

[Mike West]

(In reply to comment #10)
> Can't you turn private browsing mode on and off during runtime? In that case you might want to clear the console messages stored in the inspector when private browsing is disabled again.

That makes sense. We should also clear any stored exceptions that haven't made it to the console yet (ScriptExecutionContext::PendingException).

[Timothy Hatcher]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > They can go to the system log (seen via Console.app) if you just launch the app via double click (like a user). If you launch via the Terminal or Xcode it dumps to the TTY and not the system log.
> 
> Thanks for the detail! Are you talking about the printf'd messages, or the messages sent to the console?
> 
> I see three things happening in Console::addMessage:
> 
> 1. The message is sent to a ChromeClient.
> 2. The message is sent to InspectorInstrumentation.
> 3. The message is (potentially) printf'd.
> 
> Do all three potentially leak information to the system log? If possible, I'd like to retain #2, as I haven't seen any code in Inspector that would write information outside the context of the browsing session, but I agree that the other two should probably be gated on private browsing mode.
> 
> I'll put up a patch for discussion on Monday.

I was just talking about printf.

[Mike West]

Created attachment 178373 [details]
Patch

[Mike West]

(In reply to comment #13)
> Created an attachment (id=178373) [details]
> Patch

Here's a strawman that blocks ChromeClient and printf notifications, but allows InspectorInstrumentation. If there's agreement that this is what we want, I'll put some tests together.

Filed bug 104454 to cover clearing console state when exiting private browsing, as it's a bit tangential to what we're focusing on here.

[Mike West]

Created attachment 178537 [details]
Patch

[Mike West]

I had a quick chat with Pavel, he's happy with the change.

Carrying over Pavel's r+ and throwing the updated patch to the bots. I'll wait to land this to give PST-zone folks a chance to weigh in.

Thanks!

[WebKit Review Bot]

Comment on attachment 178537 [details]
Patch

Attachment 178537 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15239483

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Mike West]

Comment on attachment 178537 [details]
Patch

I pinged ap on IRC last night; no concerns given the folks already CCd on the bug.

(In reply to comment #17)
> (From update of attachment 178537 [details])
> Attachment 178537 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15239483
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

This runs locally without crashing. Calling it flake; let's see what the CQ says. :)

[WebKit Review Bot]

Comment on attachment 178537 [details]
Patch

Clearing flags on attachment: 178537

Committed r137267: <http://trac.webkit.org/changeset/137267>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.