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

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[erosman]

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

Comment 1

[erosman]

Issue still present in 61.0a1 (2018-04-01) (64-bit) Win

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

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 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1450186 - Clear cached messages when user clear messages; .

This is a first, naive implementation.
I don't really like how we pass the webconsoleClient,
but at the time we create the thunk middleware, the
proxy isn't on the hud yet (which we might revisit later).
Also maybe we could not pass the whole webConsoleClient, but
either the functions we need, or maybe simply a onClearMessages
callback, but things might get hard to follow.
This definitely needs a mochitests to make sure that we do remove
those messages on clear.

Review commit: https://reviewboard.mozilla.org/r/234128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234128/

Comment 4

[Code Review Bot [:reviewbot]]

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]

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234128/diff/1-2/

Comment 6

[erosman]

Issue still present in 61.0a1 (2018-04-08) (64-bit) Win

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234128/diff/2-3/

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234128/diff/3-4/

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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");

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234128/diff/4-5/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234128/diff/5-6/

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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 :-)

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

No, the test is only for the webconsole. I'll add either a dedicated test file or a test case here.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8965386 [details]
Bug 1450186 - Clear cached messages when user clear messages; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234128/diff/6-7/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15a84869eea4
Clear cached messages when user clear messages; r=Honza.

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/15a84869eea4

Comment 18

[erosman]

Not sure if it is related or not.. seems related...

Problem exists in Debug: Developer Tools

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

(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) ?

Comment 20

[erosman]

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