659625 - Implement console.clear to clear the console output

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Firebug has a console.clear() function that clears the console output, same as our clear() method. It would be nice to implement that in our web console as well.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Proposed patch

Quick implementation and test case.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

Botched renaming the test cases in the first patch. Fixed.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 535054 [details] [diff] [review]
Patch v2

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

Good quick patch.

You need a test in dom/tests/browser/browser_ConsoleAPITests.js as well.

Giving the patch r+, but please address the comments.

::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_659625_console_clear.js
@@ +6,5 @@
> +
> +// Tests that the console.clear() method call clears the console.
> +
> +const TEST_URI = "http://example.com/browser/toolkit/components/console/" +
> +                 "hudservice/tests/browser/test-bug-659625-console-clear.html";

I don't think you need a separate web page for this test. Use a data URI like data:text/html,<p>test for bug 659625 - console.clear().

@@ +30,5 @@
> +
> +  let hud = HUDService.hudReferences[gHudId];
> +  outputNode = hud.outputNode;
> +
> +  findLogEntry("start");

You can call content.console.log("start").

@@ +33,5 @@
> +
> +  findLogEntry("start");
> +  let button = content.document.querySelector("button");
> +  ok(button, "we have the button");
> +  EventUtils.sendMouseEvent({ type: "click" }, button, content);

And here you can call content.console.clear().

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

Addressed all comments by msucan. Requesting review from sdwilsh since this is a quite small one, and very similar to bug 658368.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Isn't it potentially annoying for websites to be able to clear your console? Does this allow pages to make themselves hard to debug with the console by calling setInterval(console.clear, 100)?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Isn't it potentially annoying for websites to be able to clear your console?
> Does this allow pages to make themselves hard to debug with the console by
> calling setInterval(console.clear, 100)?
Yeah, I'm not sure I like this for these reasons (and if we are exposing a new API to web content, we totally need sr).

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #6)
> (In reply to comment #5)
> > Isn't it potentially annoying for websites to be able to clear your console?
> > Does this allow pages to make themselves hard to debug with the console by
> > calling setInterval(console.clear, 100)?
> Yeah, I'm not sure I like this for these reasons (and if we are exposing a
> new API to web content, we totally need sr).

I'm not sure I understand the motivation and benefit from doing something like that. What would a malicious adversary accomplish by periodically clearing the console output? Isn't the console supposed to be used for debugging sites after all?

I should confess that I don't consider this an important API addition, since it is only implemented in Firebug. Other browsers don't provide this call (although Firebug wrote the book on the Console API). So, if it is deemed too troublesome, we might as well drop it. I'd sure like to understand the potential perils from its use, though.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> I'm not sure I understand the motivation and benefit from doing something like
> that. What would a malicious adversary accomplish by periodically clearing the
> console output? Isn't the console supposed to be used for debugging sites after
> all?
console.clear clears all messages, or just things added with console.log?

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm not sure I understand the motivation and benefit from doing something like
> > that. What would a malicious adversary accomplish by periodically clearing the
> > console output? Isn't the console supposed to be used for debugging sites after
> > all?
> console.clear clears all messages, or just things added with console.log?

All messages. Oh, I think I know what you're getting at: load evil.js but make sure it doesn't appear in the console. Not good.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I'm not sure I understand the motivation and benefit from doing something like
> > > that. What would a malicious adversary accomplish by periodically clearing the
> > > console output? Isn't the console supposed to be used for debugging sites after
> > > all?
> > console.clear clears all messages, or just things added with console.log?
> 
> All messages. Oh, I think I know what you're getting at: load evil.js but
> make sure it doesn't appear in the console. Not good.

Firebug is not susceptible to this, because they have a separate Net tab for network events. Also, IE implements console.clear(), but only clears log messages:

http://msdn.microsoft.com/en-us/library/dd565625(v=vs.85).aspx#remarks

Microsoft's approach doesn't even touch error messages. I could do the same in this patch.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> Firebug is not susceptible to this, because they have a separate Net tab for
> network events. Also, IE implements console.clear(), but only clears log
> messages:
> 
> http://msdn.microsoft.com/en-us/library/dd565625(v=vs.85).aspx#remarks
> 
> Microsoft's approach doesn't even touch error messages. I could do the same
> in this patch.

Microsoft's approach makes sense. If the reviewers agree, I would suggest doing the same: clear only the console.*() messages when console.clear() is called.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Microsoft's approach makes sense. If the reviewers agree, I would suggest doing
> the same: clear only the console.*() messages when console.clear() is called.
I would be much happier with that approach, yeah.

Comment 13

[Rob Campbell [:rc] (:robcee)]

I'm really not sure I see the benefit to this. I think group and groupEnd would be better uses of our time and provide a similar (if not better) means for developers to control output for debugging.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

It's definitely not something that we _have_ to support, it's just an easy win in our quest for full console API compatibility. I only worked on it because it was simple enough, we can always drop the patch if people find it useless.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v4

A version that only clears console.* output, leaving net, css and error messages intact. I had to do a minor refactoring in pruneConsoleOutputIfNecessary() for this, promoting it to a public method as well.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 536085 [details] [diff] [review]
Patch v4

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

r=sdwilsh with comments addressed

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +1952,5 @@
> +   *        The category of message nodes to limit.
> +   * @return number
> +   *         The current user-selected log limit.
> +   */
> +  pruneConsoleOutputIfNecessary: function HS_pruneConsoleOutputIfNecessary(

This method doesn't need to be public though, right?  Only deleteNodes needs to be exposed and this can stay local to the file (if I'm reading things right that is).

@@ +2038,5 @@
>  
>      switch (level) {
> +      case "clear":
> +        // Clear console category and input/output messages.
> +        for (let cat = 3; cat < 6; cat++) {

Please don't use magic numbers.  Pull these out into constants.

::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_659625_console_clear.js
@@ +35,5 @@
> +  content.alert(2);
> +  content.console.clear();
> +  let nodes = outputNode.querySelectorAll(".webconsole-msg-console");
> +  is(nodes.length, 0, "no nodes");
> +  // Make sure console.clear() does not remove network messages.

What about other messages?  We should really check for errors/warnings too.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v5

Updated to address the comments in comment 16.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #16)
> Comment on attachment 536085 [details] [diff] [review] [review]
> Patch v4
> 
> Review of attachment 536085 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r=sdwilsh with comments addressed
> 
> ::: toolkit/components/console/hudservice/HUDService.jsm
> @@ +1952,5 @@
> > +   *        The category of message nodes to limit.
> > +   * @return number
> > +   *         The current user-selected log limit.
> > +   */
> > +  pruneConsoleOutputIfNecessary: function HS_pruneConsoleOutputIfNecessary(
> 
> This method doesn't need to be public though, right?  Only deleteNodes needs
> to be exposed and this can stay local to the file (if I'm reading things
> right that is).

Fixed.

> @@ +2038,5 @@
> >  
> >      switch (level) {
> > +      case "clear":
> > +        // Clear console category and input/output messages.
> > +        for (let cat = 3; cat < 6; cat++) {
> 
> Please don't use magic numbers.  Pull these out into constants.

Sorry, I don't know how I forgot these. Fixed.

> :::
> toolkit/components/console/hudservice/tests/browser/
> browser_webconsole_bug_659625_console_clear.js
> @@ +35,5 @@
> > +  content.alert(2);
> > +  content.console.clear();
> > +  let nodes = outputNode.querySelectorAll(".webconsole-msg-console");
> > +  is(nodes.length, 0, "no nodes");
> > +  // Make sure console.clear() does not remove network messages.
> 
> What about other messages?  We should really check for errors/warnings too.

Added a check for errors, removed forgotten alerts (oops!) and simplified the test a bit.

Thanks!

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 536549 [details] [diff] [review]
Patch v5

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  deleteNodes: function HS_deleteNodes(aConsoleNode, aCategory, aLimit) {

>+    let hudRef = HUDService.getHudReferenceForOutputNode(aConsoleNode);

Not related to this patch, but can't this use |this| instead of HUDService? Another thing to add to the cleanup bugs perhaps.

>+        // Clear console category and input/output messages.
>+        for (let cat = CATEGORY_WEBDEV; cat <= CATEGORY_OUTPUT; cat++) {

Why clear input/output messages? Aren't those the result of the user evaluating things? I thought we only wanted to let the page clear things the page added (via window.console).

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #19)
> Comment on attachment 536549 [details] [diff] [review] [review]
> Patch v5
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  deleteNodes: function HS_deleteNodes(aConsoleNode, aCategory, aLimit) {
> 
> >+    let hudRef = HUDService.getHudReferenceForOutputNode(aConsoleNode);
> 
> Not related to this patch, but can't this use |this| instead of HUDService?
> Another thing to add to the cleanup bugs perhaps.

Yes it can, I will fix it.

> >+        // Clear console category and input/output messages.
> >+        for (let cat = CATEGORY_WEBDEV; cat <= CATEGORY_OUTPUT; cat++) {
> 
> Why clear input/output messages? Aren't those the result of the user
> evaluating things? I thought we only wanted to let the page clear things the
> page added (via window.console).

My idea was to clear as much as possible, as Firebug does, without introducing security problems. So, network requests are not touched and so are error messages, just to be on the safe side. But when I only cleared the console.* output, the input/output felt out of place and didn't seem to be of much help, either. Do you think that we ought to leave input/output alone?

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v6

Changed HUDService to |this|.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I'm still rather confused as to the utility of this method. Why would a web site/JS library/etc. want to make use of it? Do websites do that frequently? Do you have any examples?

The user of the console can already clear it themselves, so I don't understand why allowing a web site to do it would be beneficial.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to comment #22)
> I'm still rather confused as to the utility of this method. Why would a web
> site/JS library/etc. want to make use of it? Do websites do that frequently?
> Do you have any examples?
>
> The user of the console can already clear it themselves, so I don't
> understand why allowing a web site to do it would be beneficial.

Sorry, I don't know of any such examples. I've never used it myself. My reason for implementing this was feature parity with Firebug and IE.

Comment 24

[Rob Campbell [:rc] (:robcee)]

I contend that this method is not all that useful. If we're only implementing it to get a checkmark next to the list of API functions we implement, then I'm not sure it's worth doing.

Personally, I don't like having this method remove all of the contents of my console. We'd discussed in a meeting the possibility of having clear() behave as a new grouping separator between sections in the log. I'd be more accepting of that as the behavior than actually removing the nodes.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 537505 [details] [diff] [review]
Patch v6

I agree with Rob - the bar for adding new content-exposed APIs needs to be higher than "firebug does it", particularly when that API has the potential for abuse (as minor as it may be in this case).

That isn't to say that we should *never* implement this, to be clear - we just need more convincing arguments for doing it than have been presented here.

Comment 26

[Kevin Dangoor]

The use case for console.group is certainly stronger than that of console.clear in general.

FWIW, Webkit does not directly offer console.clear. However, both Safari and Chrome offer a mildly private console._commandLineAPI.clear()

http://stackoverflow.com/questions/3011600/clear-javascript-console-in-google-chrome

The fact that they don't expose console.clear directly but do expose it in this way is telling: they don't think it's enormously useful, but they also don't think it's particularly harmful.

Comment 27

[Paul Rouget [:paul]]

triage. Filter on PINKISBEAUTIFUL

Comment 28

[Kevin Dangoor]

It's worth noting that WebKit now *does* include console.clear (as does Firebug).

http://peter.sh/2012/11/week-and-month-date-pickers-csp-and-no-more-desktop-width-viewport-directive/

Comment 29

[Rob Campbell [:rc] (:robcee)]

good to know.

Comment 30

[Mihai Sucan [:msucan]]

What do we do here? wontfix / unassign panos / finish it up?

Comment 32

[Mahks]

For those that can't think of why this may be useful:
I use it for debugging, when I have sent a mass of debug info to the console and it is no longer needed I want it gone. The console gets dumped to a log file and do not need data that is not relevant.

Comment 33

[92502073]

I need this feature too...
Not only because Firebug did it...

Sometimes I have to write some javascript code to monitor my website status periodically...
it may send a request every 3 seconds, and log something to the WebConsole....
but I don't like the previous log message annoying me...
then I will try to use console.clear to clear previous log (In WebConsole/Scratchpad)...

maybe this feature will be misused by some people 
(such as: setInterval(function(){concole.clear()}, 100)?)

However, for those cracker...
they can simply use;

 setInterval(function(){console.log((new Date()).toString()+"\n"+document.body.html)}, 100);

to make "pollution" for our debug process in WebConsole

or we can use:

 for(var max_timer = setInterval(function(){/*noop*/}, 10000); max_timer>=0; max_timer--) {
     clearInterval(max_timer);
 }

to disable all setInterval :D
(It might have side effect)

so I don't think "to prevent those cracker misusing this feature" is a good reason to disable console.clear() method...

Comment 34

[Alexandre Poirot [:ochameau]]

You nominated this very old bug for devedition-40. Do you have more arguments to justify working on this again?

It looks like, now, console.clear() is available everywhere but Firefox.

Comment 35

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Alexandre Poirot [:ochameau] from comment #34)
> You nominated this very old bug for devedition-40. Do you have more
> arguments to justify working on this again?

Yes.

> It looks like, now, console.clear() is available everywhere but Firefox.

This is my argument - people used to chrome or Firebug may expect this method to be available a dn in fact have utility code they have written that uses it.

The current state 4 years down the road is that all other browsers implement this, so we should consider it for parity reasons without needing to go too far down the road of trying to imagine what web developers use it for. Comments #32 & #33 have reasonable use cases. To abstract these out, console.clear is mainly useful because it gives developers some control over what appears in the console, and in particular lets them clear the decks and focus on console messages starting at a certain point.

Gavin - given my response here and the current state of console.* I'm hoping you see this differently in 2015.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

I don't have any objection, though it seems like we should look into whether those other implementations attempt to mitigate abuse at all.

Comment 37

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #36)
> I don't have any objection, though it seems like we should look into whether
> those other implementations attempt to mitigate abuse at all.

I think the current mitigation strategy would continue working here - correct? Eg this scenario:

 - user is on evil site, is told they will get access to something cool if they open the tools and paste some code in similar to: `console.clear(); someEvilCode(); console.clear();`

In this case they should run into our warning  about pasting in unknown code before console.clear() is run the first time. Right?

Comment 38

[:Gavin Sharp [email: [e-mail]]]

No, see comment 5 for the original concern. But it's possible that is based on a misunderstanding of what console.clear() does - does it clear the whole console, or just stuff that was console.log()ged by the current page?

It's probably not a big deal either way.

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

The last version of the patch (now woefully outdated) only clears the output of console.log() and the input/output messages from console evaluation (see comment 20). Not sure what other browsers do today, but we should probably follow their lead.

Comment 40

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Panos Astithas [:past] from comment #39)
> The last version of the patch (now woefully outdated) only clears the output
> of console.log() and the input/output messages from console evaluation (see
> comment 20). Not sure what other browsers do today, but we should probably
> follow their lead.

Chrome & Webkit clear all of the output. I think this is the correct thing to do, fyi. 

I agree there is a scenario where a website developer spams console.clear() to make the site less debuggable / limit somewhat the ability to reverse engineer things, but I don't think that is a big deal. The user of the tools has the ability to use a stepping debugger and monkey-patch the console object, so they still have the upper hand.

Comment 41

[Mahks]

Is the console not limited to "devtools.hud.loglimit.console"?

If so, anyone wanting to hide logs could just spam the console so the limit clears what they want to hide. Thus the loss of log argument is not valid.

Comment 42

[Julian Descottes [:jdescottes]]

Attachment: bug659625.wip.patch

Here is a basic implementation of console.clear().

The behavior is similar to using the clear button of the webconsole UI.
Using console.clear() will call clearEvents on the ConsoleStorageAPI. 

If the console is opened when console.clear() is received, the UI will also be cleared. Tested Chrome and IE11 implementations, and that's how they do it at the moment ... but that behavior can be annoying for the user.

Current patch is obviously missing tests, prefer to get some feedback on the implementation before going further.

Brian: let me know what you think about this patch?

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8736660 [details] [diff] [review]
bug659625.wip.patch

Sorry, I won't be able to review this until next Wednesday.  Forwarding the platform changes to Andrea though.

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 8736660 [details] [diff] [review]
bug659625.wip.patch

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

I think a test is needed.

::: dom/base/Console.cpp
@@ +1154,5 @@
>  
>  METHOD(Count, "count")
>  
>  void
> +Console::Clear(JSContext* aCx)

No, all of this is wrong, sorry. For these reasons:

1. mStorage can be touched only on the main-thread.
2. mInnerID can be null in workers.
3. it's important to centralize the creation of mStorage.

What I propose is this:

1. add: METHOD(Clear, "clear"); where the other METHOD macros are.
2. in Console::ProcessCallData(), before RecordEvent() add:

if (aData->mMethodName == MethodClear) {
  nsresult rv = mStorage->ClearEvents(innerID);
  NS_WARN_IF(NS_FAILED(rv));
}

or something like that. Or alternatively, move all this code in RecordEvents() of ConsoleAPIStorage.

Comment 45

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part1.wip.patch

Andrea: thanks for having a look at the previous patch. The platform part did not get the care it deserved, sorry about that, I should have asked for mentoring.

This new patch should address your first comments. Can you let me know what you think?

Also do you have any hints on how to test this? I guess the goal here is to call console.clear() and check the ConsoleApiStorage has been emptied? (I will add integration mochitests checking console.clear() in the context of the devtools webconsole, but I assume you were referring to another kind of tests)

Thanks!

Comment 46

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part2.wip.patch

Brian: if you want to have a look, I moved the devtools related changes to a separate changeset. Can use your feedback on the implementation as well as on the behavior.

Comment 47

[Andrea Marchesini [:baku]]

Comment on attachment 8738477 [details] [diff] [review]
bug659625.part1.wip.patch

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

It's OK. but, I want to see a test too.

::: dom/base/Console.cpp
@@ +1527,5 @@
>  
> +  if (aData->mMethodName == MethodClear) {
> +    nsString innerWindowId;
> +    innerWindowId.AppendPrintf("%" PRIu64, mInnerID);
> +    nsresult rv = mStorage->ClearEvents(innerWindowId);

Don't create a new innerWindowID. Just use the existing innerID.

Comment 48

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part1.v1.patch

Thanks for the feedback. Added a test & removed the unneeded uint64_t to nsAString.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c16d679a4c8

Will ask for review if try is ok.

Comment 49

[Brian Grinstead [:bgrins]]

Comment on attachment 8738478 [details] [diff] [review]
bug659625.part2.wip.patch

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

Looks good to me, thanks.   I'm not sure what a good test to base this off of is, but maybe similar to browser_console_clear_on_reload.js but with ContentTask.spawn content.console.clear() instead of reloading the page and not worrying about the sidebar.

::: devtools/client/webconsole/webconsole.js
@@ +1284,5 @@
>          break;
>        }
> +      case "clear": {
> +        body = l10n.getStr("consoleCleared");
> +        clipboardText = body;

I don't think we need to set clipboardText since it should default to the body value (can you confirm that?)

Comment 50

[Julian Descottes [:jdescottes]]

Comment on attachment 8739426 [details] [diff] [review]
bug659625.part1.v1.patch

Try seems fine, flagging for review.

Comment 51

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part2.v1.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4b4951830c0

Thanks for the feedback Brian!

Regarding the clipboardText, small difference if we let createMessageNode fallback to the body: it will append the source and line: 
"Console was cleared. @ debugger eval code:1"

I added two tests, one checking the behavior when the clear command is triggered from the console, one when it's triggered from a script. I kept the part related to the sidebar, since it should be closed when receiving a console.clear() message.

Comment 52

[Brian Grinstead [:bgrins]]

Comment on attachment 8740006 [details] [diff] [review]
bug659625.part2.v1.patch

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

I'll take one more look at this after a response on the comment on the test

::: devtools/client/webconsole/test/browser_console_clear_from_webconsole.js
@@ +14,5 @@
> +add_task(function* () {
> +  yield loadTab(TEST_URI);
> +
> +  let hud = yield openConsole();
> +  ok(hud, "Web Console opened");

I don't think we need this test - We have plenty of tests that confirm that jsterm evaluations act the same as page evals.  So unless if I'm missing something (and let me know if I am), then if the 'sidebar closing' and 'closing and reopening the console' assertions here moved into browser_console_clear_from_script.js we could get away with one less test

Comment 53

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part2.v2.patch

The goal of the other test was to check that writing console.clear() in the webconsole was not purely handled on the UI side but actually executed on the backend and clearing messages in the ConsoleAPIStorage.

It's true that this is not specific to the clear method and is probably tested somewhere else.

Comment 54

[Brian Grinstead [:bgrins]]

Comment on attachment 8740289 [details] [diff] [review]
bug659625.part2.v2.patch

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

Looks good to me, thanks

::: devtools/client/webconsole/test/browser_console_clear_method.js
@@ +17,5 @@
> +  ok(hud, "Web Console opened");
> +
> +  info("Check the console.clear() done on page load has been processed.");
> +  yield waitForLog("Console was cleared", hud);
> +  ok(hud.outputNode.textContent.indexOf("Console was cleared") != -1,

Throughout this file - you can use .includes() instead of indexOf

@@ +48,5 @@
> +
> +  // Cannot wait for "Console was cleared" here because such a message is
> +  // already present and would yield immediately.
> +  yield sidebarClosed;
> +  ok(true, "Variables view sidebar closed after console.clear()");

No need for this true assertion, you could change it to go before the yield sidebarClosed as an info("Waiting for sidebar to close after console.clear()") or similar to get some extra tracing in the test logs

Comment 55

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part2.v3.patch

Thanks for the review! New version of the patch (carry over r+)
 
Will land if try is green : https://treeherder.mozilla.org/#/jobs?repo=try&revision=15edfa6e1dbe

Comment 56

[Julian Descottes [:jdescottes]]

Attachment: bug659625.part2.v3.patch

And of course I made a mistake in the test. New try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b8939122ad !

Comment 57

[Julian Descottes [:jdescottes]]

Try looks green, landing.

I think we should add "clear" to the methods listed on:
- https://developer.mozilla.org/en/docs/Web/API/console

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/89cfa8c724e1
https://hg.mozilla.org/integration/fx-team/rev/671011b82100

Comment 59

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8617963&repo=fx-team

Comment 60

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/3d003731a97b
https://hg.mozilla.org/integration/fx-team/rev/7d0dc8bacb93

Comment 61

[Julian Descottes [:jdescottes]]

Thanks for the backout!

It looks like on Android, ConsoleAPIStorage.getEvents() has 5 events when the test starts. Those events are not removed by calling console.clear().

Currently investigating what kind of console events they are to be sure it's fine that they are not removed by console.clear. After that, the fix will most likely be to use the initial amount of events as a base offset in the test, instead of assuming a clean starting state.

Comment 62

[Andrea Marchesini [:baku]]

> It looks like on Android, ConsoleAPIStorage.getEvents() has 5 events when
> the test starts. Those events are not removed by calling console.clear().

I suggest to use nsIConsoleAPIStorage.clearEvents() before testing anything else.

Comment 63

[Julian Descottes [:jdescottes]]

(In reply to Andrea Marchesini (:baku) from comment #62)
> > It looks like on Android, ConsoleAPIStorage.getEvents() has 5 events when
> > the test starts. Those events are not removed by calling console.clear().
> 
> I suggest to use nsIConsoleAPIStorage.clearEvents() before testing anything
> else.

Good call! The logs were coming from another windowId, which is why they were not removed by console.clear(). Calling clearEvents() before starting the test consequently fixes the issue.

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a56ae641acdc
https://hg.mozilla.org/integration/fx-team/rev/ff3f8e8e89dc

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a56ae641acdc
https://hg.mozilla.org/mozilla-central/rev/ff3f8e8e89dc

Comment 66

[Will Bamberg [:wbamberg]]

I've added a page on console.clear: https://developer.mozilla.org/en-US/docs/Web/API/Console/clear and referred to it from the devtools page: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Console_messages#Logging .

Please let me know if I need anything else for this!

Comment 67

[Julian Descottes [:jdescottes]]

(In reply to Will Bamberg [:wbamberg] from comment #66)
> I've added a page on console.clear:
> https://developer.mozilla.org/en-US/docs/Web/API/Console/clear and referred
> to it from the devtools page:
> https://developer.mozilla.org/en-US/docs/Tools/Web_Console/
> Console_messages#Logging .
> 
> Please let me know if I need anything else for this!

Perfect, thanks for documenting this!

Comment 68

[BoffinBrain]

I just read about this feature in the Fx 48 release notes and immediately had my reservations about it. I see that security was briefly mentioned at the start of this tracker, but it seems that the solutions have been watered down or forgotten before release. I just tested it: JS errors, security warnings and even SECURITY ERRORS are cleared - not just debug statements!

I would really like to see this addressed. Please add a way for developers to work around malicious scripts and trolling with the use of console.clear. This could be done by:

1. Allowing the user to restore the console history that was there before the console was cleared. Perhaps along with the current message "Console was cleared." provide a link you can click to restore it.
2. Do the opposite of the above: print to the console "A script attempted to clear the console history." along with a link to allow this to happen (and then remember this permission on a per-site basis).
3. Provide a global option in the Dev Console Toolbox Options panel to enable/disable console.clear.
4. Do what was suggested above: make it fold all previous lines into a group and collapse the group. (comment, #13, comment #24, comment #26)

Options 3 and 4 are probably the easiest, but I really hope this is considered before web developers everywhere need to start adding console.clear=function(){}; to their pages.

Comment 69

[Brian Grinstead [:bgrins]]

(In reply to BoffinbraiN from comment #68)
> I just read about this feature in the Fx 48 release notes and immediately
> had my reservations about it. I see that security was briefly mentioned at
> the start of this tracker, but it seems that the solutions have been watered
> down or forgotten before release. I just tested it: JS errors, security
> warnings and even SECURITY ERRORS are cleared - not just debug statements!
> 
> I would really like to see this addressed. Please add a way for developers
> to work around malicious scripts and trolling with the use of console.clear.

I really like the idea of limiting the types of messages that get cleared to be only console API messages and not service messages like security warnings.  For example, on https://people.mozilla.org/~tvyas/mixedboth.html the 'console was cleared.' message should still show up, but none of the existing messages around mixed content warnings would actually be removed.  It looks like that was the original approach i.e. Comment 20.  Would you mind filing a bug for this so we can track that work?

If we had that, I don't think anything further is needed.  Chrome has supported this for a long time, and if it's only removing messages that were logged through the console methods (and maybe input/output from console) the risk for abuse seems low.

Comment 70

[Brian Grinstead [:bgrins]]

I went ahead and filed bug 1281184 for that

Comment 71

[Julian Descottes [:jdescottes]]

You might also want to check Bug 1267856, which is about allowing developers to bypass/disable console.clear.  
At the moment the preferred approach is to add a way to filter in/out the "cleared" console messages.

(for the record, I think most browsers clear all messages regardless of their origin, Chrome included)

Comment 72

[BoffinBrain]

Thanks, Brian and Julian. I've subscribed and voted for both trackers.