Closed Bug 1450186 Opened 6 years ago Closed 6 years ago

Browser Console 'clear' removes messages from the ui but they reappear when reopening in 61.0a1 (2018-03-29) (64-bit) Win

Categories

(DevTools :: Console, defect, P1)

Product:
DevTools
Component:
Console
Version:
61 Branch
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: eros_uk, Assigned: nchevobbe)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

Bug 1450186 - Clear cached messages when user clear messages; .
59 bytes, text/x-review-board-request
Honza
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180329100042

Steps to reproduce:

Nightly 61.0a1 (2018-03-29) (64-bit) Win


After opening the Browser console (Ctrl + Shift + J) and clearing (Bin) it clears but once closed and reopened, the entries are still there.

The issue was not present in previous versions e.g. 61.0a1 (2018-03-26) (64-bit) Win and is  new in (2018-03-29) release.

errors from:
markup.js:157:7
ThreadSafeDevToolsUtils.js:88:5
etc
Component: Untriaged → Developer Tools: Console
Blocks: 1362023
Summary: Brouwer Console does not Clear in 61.0a1 (2018-03-29) (64-bit) Win → Brouwer Console 'clear' removes messages from the ui but they reappear when reopening in 61.0a1 (2018-03-29) (64-bit) Win
Priority: -- → P2
Whiteboard: [newconsole-mvp]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Brouwer Console 'clear' removes messages from the ui but they reappear when reopening in 61.0a1 (2018-03-29) (64-bit) Win → Browser Console 'clear' removes messages from the ui but they reappear when reopening in 61.0a1 (2018-03-29) (64-bit) Win
Issue still present in 61.0a1 (2018-04-01) (64-bit) Win
We used to clear the cache (and network messages) in the old frontend [1], but we don't in the new frontend.
Here we should call `webConsoleClient.clearNetworkRequests` and  `webConsoleClient.clearMessagesCache` in the MESSAGES_CLEAR action [2].

[1] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/webconsole/jsterm.js#998-1001
[2] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/webconsole/new-console-output/actions/messages.js#54-58
Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

https://reviewboard.mozilla.org/r/234128/#review239754


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/new-console-output/actions/messages.js:61
(Diff revision 1)
> +    const webConsoleClient = getWebConsoleClient ? getWebConsoleClient() : null;
> +    if (webConsoleClient) {
> +      webConsoleClient.clearNetworkRequests();
> +      webConsoleClient.clearMessagesCache();
> +    } else {
> +      console.warn("no webconsoleClient")

Error: Missing semicolon. [eslint: semi]
Issue still present in 61.0a1 (2018-04-08) (64-bit) Win
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

https://reviewboard.mozilla.org/r/234128/#review240924

Thanks for the patch, looks great to me!

Just two minor inline comments.

R+ assuming try is green

Honza

::: devtools/client/webconsole/test/mochitest/browser_webconsole_clear_cache.js:5
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Check that clearing the output also clear the console cache.

nit: ...clears the console cache.

::: devtools/client/webconsole/test/mochitest/browser_webconsole_clear_cache.js:20
(Diff revision 4)
> +  const CACHED_MESSAGE = "CACHED_MESSAGE";
> +  let onMessage = waitForMessage(hud, CACHED_MESSAGE);
> +  ContentTask.spawn(gBrowser.selectedBrowser, CACHED_MESSAGE, function(str) {
> +    content.wrappedJSObject.console.log(str);
> +  });
> +  await onMessage;

I am seeing this pattern 4x in the test. Perhaps there could be a helper used as follows?

await waitForCustomMessage(hud, "CACHED_MESSAGE");
await waitForCustomMessage(hud, "SMOKE_MESSAGE");
await waitForCustomMessage(hud, "NEW_CACHED_MESSAGE");
await waitForCustomMessage(hud, "SMOKE_MESSAGE");
Attachment #8965386 - Flags: review?(odvarko) → review+
Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Clearing the review flag since I changed the approach of the patch because console.clear wasn't working anymore (see [1]) and some tests were failing.

We could have made a workaround for batching + thunk, but I feel like a middleware is even better that what we were doing before since it nicely encapsulate "side effects" (cache clearing)

Let me know if you have concerns about this.

---

[1] In https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/actions/messages.js#36-42 we handle console.clear by batching 2 actions: clearing and adding a new message. Making the messagesClear action a thunk, this wasn't working as expected anymore
Attachment #8965386 - Flags: review+ → review?(odvarko)
Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

https://reviewboard.mozilla.org/r/234128/#review241256

Thanks Nicolas, looks great to me!

I assume that the test is covering both the Console panel as well as the Browser console, correct?

R+ assuming try is green

Honza

::: devtools/client/webconsole/store.js:79
(Diff revision 6)
>      compose(
>        applyMiddleware(thunk.bind(null, {prefsService})),
>        enableActorReleaser(hud),
>        enableBatching(),
> -      enableNetProvider(hud)
> +      enableNetProvider(hud),
> +      enableMessagesCacheClearing(hud),

I really like using middleware for this. It's nicely consistent with the other cases (net provider and actor releaser) that also deal with the backend. 

I think that in general any logic that is changing/syncing data on the backend (i.e. not in the client store) can be done through the same technique. 

We might also see some patterns over time and come up with some helpers (or more sophisticated) architecture (and perhps shared across panels).

::: devtools/client/webconsole/store.js:258
(Diff revision 6)
>      return next(netProviderEnhancer, initialState, enhancer);
>    };
>  }
> +
> +/**
> + * This enhancer is responsible for clearing the messages caches using the

nit: ...clearing the messages cache...

::: devtools/client/webconsole/test/mochitest/browser_webconsole_clear_cache.js:13
(Diff revision 6)
> +
> +const TEST_URI = "data:text/html;charset=utf8,Test clear cache";
> +
> +add_task(async function() {
> +  const tab = await addTab(TEST_URI);
> +  let hud = await openConsole(tab);

Nice, `const` is everywhere :-)
Attachment #8965386 - Flags: review?(odvarko) → review+
No, the test is only for the webconsole. I'll add either a dedicated test file or a test case here.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15a84869eea4
Clear cached messages when user clear messages; r=Honza.
https://hg.mozilla.org/mozilla-central/rev/15a84869eea4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Not sure if it is related or not.. seems related...

Problem exists in Debug: Developer Tools
(In reply to erosman from comment #18)
> Not sure if it is related or not.. seems related...
> 
> Problem exists in Debug: Developer Tools

What do you mean ? The issue described in Comment 0 was fixed (and a test was added to make sure of that).
Do you still see the issue in 61.0a1 (2018-04-12) ?
You are right ..... I was testing with 61.0a1 (2018-04-08) (64-bit) Win
It is also fixed in the Debug: Developer Tools Console
Tested on 61.0a1 (2018-04-13) (64-bit) Win
Product: Firefox → DevTools
Depends on: 1469098
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: