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?
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.
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.
+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
> 3. What about the strange printf'ing we do in Console::addMessage? Is that necessary? It implements WebKit1 Mac SPI, +[WebCoreStatistics setShouldPrintExceptions:].
(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.
(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?
(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.
(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.
(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).
(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.
Created attachment 178373 [details] Patch
(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.
Created attachment 178537 [details] Patch
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!
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
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. :)
Comment on attachment 178537 [details] Patch Clearing flags on attachment: 178537 Committed r137267: <http://trac.webkit.org/changeset/137267>
All reviewed patches have been landed. Closing bug.