Closed Bug 659625 Opened 13 years ago Closed 8 years ago

Implement console.clear to clear the console output

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: past, Assigned: jdescottes)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [console-papercuts][polish-backlog])

Attachments

(2 files, 12 obsolete files)

7.84 KB, patch
baku
: review+
Details | Diff | Splinter Review
11.94 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → past
Severity: normal → enhancement
Status: NEW → ASSIGNED
Attached patch Proposed patch (obsolete) — Splinter Review
Quick implementation and test case.
Attachment #535050 - Flags: review?(mihai.sucan)
Attached patch Patch v2 (obsolete) — Splinter Review
Botched renaming the test cases in the first patch. Fixed.
Attachment #535050 - Attachment is obsolete: true
Attachment #535050 - Flags: review?(mihai.sucan)
Attachment #535054 - Flags: review?(mihai.sucan)
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().
Attachment #535054 - Flags: review?(mihai.sucan) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Addressed all comments by msucan. Requesting review from sdwilsh since this is a quite small one, and very similar to bug 658368.
Attachment #535054 - Attachment is obsolete: true
Attachment #535146 - Flags: review?(sdwilsh)
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)?
(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).
(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.
(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?
(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.
(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.
(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.
(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.
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.
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.
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #535146 - Attachment is obsolete: true
Attachment #535146 - Flags: review?(sdwilsh)
Attachment #536085 - Flags: review?(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.
Attachment #536085 - Flags: review?(sdwilsh) → review+
Attachment #536085 - Flags: superreview?(gavin.sharp)
Attached patch Patch v5 (obsolete) — Splinter Review
Updated to address the comments in comment 16.
Attachment #536085 - Attachment is obsolete: true
Attachment #536085 - Flags: superreview?(gavin.sharp)
Attachment #536549 - Flags: superreview?(gavin.sharp)
(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 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).
(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?
Attached patch Patch v6 (obsolete) — Splinter Review
Changed HUDService to |this|.
Attachment #536549 - Attachment is obsolete: true
Attachment #536549 - Flags: superreview?(gavin.sharp)
Attachment #537505 - Flags: superreview?(gavin.sharp)
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.
(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.
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 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.
Attachment #537505 - Flags: superreview?(gavin.sharp) → superreview-
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.
triage. Filter on PINKISBEAUTIFUL
Component: Developer Tools → Developer Tools: Console
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/
good to know.
Priority: -- → P3
Whiteboard: [console][console-output][console-api]
What do we do here? wontfix / unassign panos / finish it up?
Blocks: 922204
Assignee: past → nobody
Status: ASSIGNED → NEW
Summary: Expand console object with clear method → Implement console.clear to clear the console output
Whiteboard: [console][console-output][console-api] → [console-papercuts]
Depends on: 1139794
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.
Whiteboard: [console-papercuts] → [console-papercuts][devedition-40]
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...
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.
Flags: needinfo?(jgriffiths)
(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.
Flags: needinfo?(jgriffiths) → needinfo?(gavin.sharp)
I don't have any objection, though it seems like we should look into whether those other implementations attempt to mitigate abuse at all.
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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?
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.
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.
(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.
Whiteboard: [console-papercuts][devedition-40] → [console-papercuts][polish-backlog]
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.
Priority: P3 → P1
Assignee: nobody → jdescottes
Attached patch bug659625.wip.patch (obsolete) — Splinter Review
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?
Attachment #8736660 - Flags: feedback?(bgrinstead)
Status: NEW → ASSIGNED
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.
Attachment #8736660 - Flags: review?(amarchesini)
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.
Attachment #8736660 - Flags: review?(amarchesini) → review-
Attached patch bug659625.part1.wip.patch (obsolete) — Splinter Review
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!
Attachment #8738477 - Flags: feedback?(amarchesini)
Attached patch bug659625.part2.wip.patch (obsolete) — Splinter Review
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.
Attachment #8736660 - Attachment is obsolete: true
Attachment #8736660 - Flags: feedback?(bgrinstead)
Attachment #8738478 - Flags: feedback?(bgrinstead)
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.
Attachment #8738477 - Flags: feedback?(amarchesini)
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.
Attachment #537505 - Attachment is obsolete: true
Attachment #8738477 - Attachment is obsolete: true
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?)
Attachment #8738478 - Flags: feedback?(bgrinstead) → feedback+
Comment on attachment 8739426 [details] [diff] [review]
bug659625.part1.v1.patch

Try seems fine, flagging for review.
Attachment #8739426 - Flags: review?(amarchesini)
Attachment #8739426 - Flags: review?(amarchesini) → review+
Attached patch bug659625.part2.v1.patch (obsolete) — Splinter Review
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.
Attachment #8738478 - Attachment is obsolete: true
Attachment #8740006 - Flags: review?(bgrinstead)
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
Attachment #8740006 - Flags: review?(bgrinstead)
Attached patch bug659625.part2.v2.patch (obsolete) — Splinter Review
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.
Attachment #8740006 - Attachment is obsolete: true
Attachment #8740289 - Flags: review?(bgrinstead)
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
Attachment #8740289 - Flags: review?(bgrinstead) → review+
Attached patch bug659625.part2.v3.patch (obsolete) — Splinter Review
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
Attachment #8740289 - Attachment is obsolete: true
Attachment #8740502 - Flags: review+
And of course I made a mistake in the test. New try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b8939122ad !
Attachment #8740502 - Attachment is obsolete: true
Attachment #8740530 - Flags: review+
Try looks green, landing.

I think we should add "clear" to the methods listed on:
- https://developer.mozilla.org/en/docs/Web/API/console
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8617963&repo=fx-team
Flags: needinfo?(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.
Flags: needinfo?(jdescottes)
> 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.
(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.
https://hg.mozilla.org/mozilla-central/rev/a56ae641acdc
https://hg.mozilla.org/mozilla-central/rev/ff3f8e8e89dc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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!
Flags: needinfo?(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!
Flags: needinfo?(jdescottes)
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.
(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.
I went ahead and filed bug 1281184 for that
Blocks: 1281184
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)
Thanks, Brian and Julian. I've subscribed and voted for both trackers.
Blocks: 1296870
Depends on: 1335487
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: