Closed Bug 1307928 Opened 8 years ago Closed 6 years ago

Support private context exited messages

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: linclark, Assigned: nchevobbe)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(4 files)

Bug 1307928 - Add a "private" property to the ConsoleMessage type; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307928 - Remove private messages on lastPrivateContextExited event; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307928 - Add mocha tests for private messages; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307928 - Add mocha tests for NewConsoleOutputWrapper queues; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Priority: -- → P2
Whiteboard: new-console
STR:
Open browser console
Open private window and load `data:text/html,<script>foo()</script>`
Notice errors in BC
Close private window
Notice errors go away in BC
Blocks: enable-new-console
Flags: qe-verify+
QA Contact: iulia.cristescu
Whiteboard: new-console → [new-console]
(In reply to Brian Grinstead [:bgrins] from comment #1)
> STR:
> Open browser console
> Open private window and load `data:text/html,<script>foo()</script>`
> Notice errors in BC
> Close private window
> Notice errors go away in BC

For some reason I can't make this work on Dev edition, but indeed it works in release.
After checking with Brian, the feature is currently working only in non-e10s.
The test covering it is browser_console_private_browsing.js and is skipped in e10s (tracked in Bug 1243960).
(In reply to Julian Descottes [:jdescottes] from comment #3)
> After checking with Brian, the feature is currently working only in non-e10s.
> The test covering it is browser_console_private_browsing.js and is skipped
> in e10s (tracked in Bug 1243960).

To be sure, it's not even working with the old frontend in e10s (and the browser console is always using the old frontend for now).  In the new frontend we don't support it at all yet.
Whiteboard: [new-console] → [console-html]
Priority: P2 → P3
Whiteboard: [console-html] → [reserve-console-html]
Priority: P3 → P4
Flags: qe-verify+
Priority: P4 → P2
QA Contact: iulia.cristescu
Whiteboard: [reserve-console-html] → [newconsole-mvp]
Blocks: 1403188
So, the `lastPrivateContextExited` event we listen for is still emited, but it looks like there is no message marked as `private` anymore (a `private` attribute was set on the message node).
This attribute is set here for example: https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/devtools/client/webconsole/webconsole.js#1334-1336.

So if it is not set, it's because the `message.private` property in the packet is falsy (undefined from what I saw while debugging).

If I'm correct, the `private` property is set https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/dom/console/Console.cpp#1534 .

baku, would you know why this does not work in e10s ?
Flags: needinfo?(amarchesini)
I confirm that message.private is correctly set in e10s and non-e10s.

> https://searchfox.org/mozilla-central/rev/
> 5536f71c3833018c4f4e2c73f37eae635aab63ff/devtools/client/webconsole/
> webconsole.js#1334-1336.

It seems that this code is not used anymore in the new webconsole.
Doing a quick debugging, I see that devtools/client/webconsole/new-console-output/utils/messages.js transformConsoleAPICallPacket() ignores message.private.

Should we have a 'private' attribute in ConsoleMessage?
Flags: needinfo?(amarchesini) → needinfo?(nchevobbe)
You're right, sorry, I was probably looking at the wrong place. And yes, we should have the private property on ConsoleMessage.
Flags: needinfo?(nchevobbe)
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
So, I double checked, and it seems that the private property is set on the packet, but only for the webconsole in a private window.
Here's the 2 packets I get when I do a console.debug call in a private content tab


Webconsole: 

```json
{
  "from": "server1.conn3.child1/consoleActor2",
  "type": "consoleAPICall",
  "message": {
    "addonId": "",
    "arguments": [
      "Console.debug",
      48.34233224436388
    ],
    "columnNumber": 22,
    "counter": null,
    "filename": "https://nchevobbe.github.io/demo/console-test-app.html",
    "functionName": "handleLog",
    "groupName": "",
    "level": "debug",
    "lineNumber": 205,
    "prefix": "",
    "private": true,
    "styles": [],
    "timeStamp": 1519825497861,
    "timer": null,
    "workerType": "none",
    "category": "webdev"
  }
}
```


Browser console: 

```json
{
  "from": "server1.conn2.consoleActor2",
  "type": "consoleAPICall",
  "message": {
    "level": "debug",
    "filename": "https://nchevobbe.github.io/demo/console-test-app.html",
    "lineNumber": 205,
    "functionName": "handleLog",
    "timeStamp": 1519825497861,
    "addonId": "",
    "arguments": [
      "Console.debug",
      48.34233224436388
    ],
    "workerType": "none",
    "styles": [],
    "category": "webdev"
  }
}
```

Not sure what's happening here
Here lies the issue: https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/devtools/server/actors/webconsole/content-process-forward.js#54-63 

Basically, in this we forward content process logs to the browser console / toolbox. 
BUT, we do re-create the message data, and in doing so, we omit some of the properties, including `private` (and `prefix`, `counter`, …).

This can be fixed by only overriding what need to be overridden: 
```js
let msgData = {
  ..consoleMsg,
  arguments: [],
  filename: consoleMsg.filename.substring(0, MSG_MGR_CONSOLE_INFO_MAX),
  functionName: consoleMsg.functionName && consoleMsg.functionName.substring(0, MSG_MGR_CONSOLE_INFO_MAX),
};
```
Depends on: 1441864
For errors, it seems that the private property is also always set to false (See Bug 1443079).
Depends on: 1443079
Removing the blocker to address it in a error dedicated follow-up
No longer depends on: 1443079
Comment on attachment 8956103 [details]
Bug 1307928 - Add a "private" property to the ConsoleMessage type; .

https://reviewboard.mozilla.org/r/225042/#review231014
Attachment #8956103 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956104 [details]
Bug 1307928 - Remove private messages on lastPrivateContextExited event; .

https://reviewboard.mozilla.org/r/225044/#review231016

So if I'm following this correctly: when one private window exits we remove all the messages from every private window? Doesn't this mean we unnecessarily remove messages from PW 1 when PW 2 gets closed? If that's the behavior in the old UI as well then let's stick with it here to avoid complicating things, but I'm thinking we may want to ultimately change the packet format so we get `private: (int) privateWindowID` or something, and then change isPrivate checks to also take in the window id that was closed. I don't think that will change the overall approach we take here, but what do you think?

Also, the diff in the reducer is a little hard to follow in mozreview - I think it'd have been easier if the `cleanState` split and some of the jsdoc / helper function cleanup happened in a previous changeset and then the changes to cleanState necessary to support the private messages feature happened in this one. But, no need to split it up at this point unless if we need to make significant changes to it.

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:293
(Diff revision 1)
> +      const queuedNetworkMessage = this.queuedMessageAdds.find(p => p.actor === actor);
> +      if (queuedNetworkMessage && isPacketPrivate(queuedNetworkMessage)) {
> +        return false;
> +      }
> +
> +      const requestMessage = [...getAllMessagesById(store.getState()).entries()]

Should `[...getAllMessagesById(store.getState()).entries()]` be done outside this loop? I'm not sure:

1) how many queued message updates we expect to have
2) how long moving all the messages into an array typically takes for various state sizes

But if either of those numbers is big it may be worth optimizing. Although since this action won't happen that it may not matter much.

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:305
(Diff revision 1)
> +    });
> +
> +    // For (network) requests updates, we can check only the state, since there must be a
> +    // user interaction to get an update (i.e. the network message is displayed and thus
> +    // in the state).
> +    this.queuedRequestUpdates = this.queuedMessageUpdates.filter(packet => {

Shouldn't this filter `queuedRequestUpdates` and not `queuedMessageUpdates`?

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:314
(Diff revision 1)
> +      }
> +
> +      return true;
> +    });
> +
> +    this.queuedMessageAdds = this.queuedMessageAdds.filter(p => !isPacketPrivate(p));

Nit: can you move this up to the beginning of the function since it's the most simple filter and we can get it out of the way early?

::: devtools/client/webconsole/new-console-output/reducers/messages.js:458
(Diff revision 1)
> + *
> + * @param {MessageState} state
> + * @param {Array} removedMessagesIds
> + * @returns {MessageState}
> + */
> +function cleanState(state, removedMessagesIds) {

It looks like all the cleanup that happens in this function is dealing updating it to match up with the now-removed messages being gone. So it seems to me this function should be named something like `removeMessagesFromState` or just `removeMessages`.
Attachment #8956104 - Flags: review?(bgrinstead)
Thanks for the review Brian, I'll address the comment shortly

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8956104 [details]
> Bug 1307928 - Remove private messages on lastPrivateContextExited event; .
> 
> https://reviewboard.mozilla.org/r/225044/#review231016
> 
> So if I'm following this correctly: when one private window exits we remove
> all the messages from every private window? Doesn't this mean we
> unnecessarily remove messages from PW 1 when PW 2 gets closed? If that's the
> behavior in the old UI as well then let's stick with it here to avoid
> complicating things

So here we only react to the "lastPrivateContextExited" event, which is emitted when **all** the private windows are closed. This is indeed what the old frontend did (the message elements were added with a `private` attribute, and when lastPrivateContextExited was received, we were removing all the `[private]` messages.
 
> but I'm thinking we may want to ultimately change the
> packet format so we get `private: (int) privateWindowID` or something, and
> then change isPrivate checks to also take in the window id that was closed.
> I don't think that will change the overall approach we take here, but what
> do you think?

A by-window solution would be better, I agree.

> :::
> devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:
> 293
> (Diff revision 1)
> > +      const queuedNetworkMessage = this.queuedMessageAdds.find(p => p.actor === actor);
> > +      if (queuedNetworkMessage && isPacketPrivate(queuedNetworkMessage)) {
> > +        return false;
> > +      }
> > +
> > +      const requestMessage = [...getAllMessagesById(store.getState()).entries()]
> 
> Should `[...getAllMessagesById(store.getState()).entries()]` be done outside
> this loop? I'm not sure:
> 
> 1) how many queued message updates we expect to have
> 2) how long moving all the messages into an array typically takes for
> various state sizes
> 
> But if either of those numbers is big it may be worth optimizing. Although
> since this action won't happen that it may not matter much.

I don't expect the queues to have too much data, but I'll check if we can optimize the flush

> :::
> devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:
> 305
> (Diff revision 1)
> > +    });
> > +
> > +    // For (network) requests updates, we can check only the state, since there must be a
> > +    // user interaction to get an update (i.e. the network message is displayed and thus
> > +    // in the state).
> > +    this.queuedRequestUpdates = this.queuedMessageUpdates.filter(packet => {
> 
> Shouldn't this filter `queuedRequestUpdates` and not `queuedMessageUpdates`?

Right. Which also mean I should test that those queues are cleaned as expected.
Comment on attachment 8956105 [details]
Bug 1307928 - Add mocha tests for private messages; .

https://reviewboard.mozilla.org/r/225046/#review231304

This looks pretty good - just going to clear review until the prior patch gets re-pushed since it may require some test changes
Attachment #8956105 - Flags: review?(bgrinstead)
Comment on attachment 8956104 [details]
Bug 1307928 - Remove private messages on lastPrivateContextExited event; .

https://reviewboard.mozilla.org/r/225044/#review231726

::: commit-message-3d250:4
(Diff revision 2)
> +Bug 1307928 - Remove private messages on lastPrivateContextExited event; r=bgrins.
> +
> +Add a redux action to handle the event and clear private messages from the store.
> +A new `cleanState` function was extraced from `limitTopLevelMessage` so we can

Typo: "extracted"

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:314
(Diff revision 2)
> +      }
> +
> +      return true;
> +    });
> +
> +    // Finalyy we clear the messages queue. This needs to be done here since we use it to

Typo: "Finally"

::: devtools/client/webconsole/new-console-output/reducers/messages.js:214
(Diff revision 2)
> +          removedIds.push(id);
> +        }
> +      }
> +
> +      // If there's no private messages, there's no need to change the state.
> +      if (removedIds.length === 0) {

I wonder if this condition should be handled inside the `removeMessagesFromState` function as an immediate return if there isn't anything to remove. There would be one extra clone of `...state` but we wouldn't have to worry about adding this extra check for other callers. What do you think?
Attachment #8956104 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956105 [details]
Bug 1307928 - Add mocha tests for private messages; .

https://reviewboard.mozilla.org/r/225046/#review232196

Glad this is a unit test and not mochitest-browser (as it would have likely been in the old frontend).

::: devtools/client/webconsole/new-console-output/test/store/private-messages.test.js:45
(Diff revision 2)
> +  let actions;
> +  before(() => {
> +    actions = setupActions();
> +  });
> +
> +  it("does not group private and non-private messages", () => {

"does not group" -> "does not repeat"

::: devtools/client/webconsole/new-console-output/test/store/private-messages.test.js:58
(Diff revision 2)
> +    let state = getState();
> +    expect(getAllMessagesById(state).size).toBe(2);
> +    expect(Object.keys(getAllRepeatById(state)).length).toBe(0);
> +  });
> +
> +  it("does group private messages", () => {

"does group" -> "does repeat"

::: devtools/client/webconsole/new-console-output/test/store/private-messages.test.js:92
(Diff revision 2)
> +    state = getState();
> +    expect(getAllMessagesById(state).size).toBe(1);
> +    expect(getVisibleMessages(state).length).toBe(1);
> +  });
> +
> +  it("cleans messagesUiById on PRIVATE_MESSAGES_CLEAR action", () => {

I'm not sure if having a separate function for this is worth the boilerplate vs checking `getAllMessagesUiById` alongside `getAllMessagesById` and `getVisibleMessages` in the function above. Up to you.

::: devtools/client/webconsole/new-console-output/test/store/private-messages.test.js:109
(Diff revision 2)
> +
> +    state = getState();
> +    expect(getAllMessagesUiById(state).length).toBe(1);
> +  });
> +
> +  it("cleans repeatsById on PRIVATE_MESSAGES_CLEAR action", () => {

Similarly, I think this test case covers `"does group private messages"` assertions (and then some) - up to you if you want to keep that one or delete it.

::: devtools/client/webconsole/new-console-output/test/store/private-messages.test.js:209
(Diff revision 2)
> +    networkUpdates = getAllNetworkMessagesUpdateById(getState());
> +    expect(Object.keys(networkUpdates)).toEqual([publicMessageId]);
> +  });
> +
> +  it("releases private backend actors on PRIVATE_MESSAGES_CLEAR action", () => {
> +    let releasedActors = [];

There's a lot of boilerplate to do this. I noticed we have do this in a few other places as well: https://searchfox.org/mozilla-central/search?q=const+%7B+dispatch%2C+getState+%7D+%3D+setupStore%28%5B%5D%2C+%7B&path=

Doesn't have to be done here, but worth considering / following up if we can make our setupStore helper function better support these use cases.
Attachment #8956105 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956762 [details]
Bug 1307928 - Add mocha tests for NewConsoleOutputWrapper queues; .

https://reviewboard.mozilla.org/r/225728/#review231730

::: devtools/client/webconsole/new-console-output/test/components/new-console-output-wrapper.test.js:34
(Diff revision 1)
> +    }
> +  };
> +  return new NewConsoleOutputWrapper(null, jsterm);
> +}
> +
> +function getPrivatePacket(key) {

This was used in the last test as well - can this be pulled out into a helper function?

::: devtools/client/webconsole/new-console-output/test/components/new-console-output-wrapper.test.js:88
(Diff revision 1)
> +      ncow.getStore().dispatch(messagesAdd([
> +        stubPackets.get("GET request"),
> +        {...getPrivatePacket("XHR GET request"), actor: getId},
> +      ]));
> +
> +      // Addpacket to the message queue to make sure that update to private requests are

Typo: "Add packet"

::: devtools/client/webconsole/new-console-output/test/components/new-console-output-wrapper.test.js:126
(Diff revision 1)
> +      ncow.queuedRequestUpdates.push(
> +        {id: publicId},
> +        {id: privateXhrGetId},
> +        {id: privateXhrPostId},
> +      );
> +      // ncow.queuedRequestUpdates.push({fakePacket: "request-update"});

Was this line meant to be removed?
Attachment #8956762 - Flags: review?(bgrinstead) → review+
Comment on attachment 8956762 [details]
Bug 1307928 - Add mocha tests for NewConsoleOutputWrapper queues; .

https://reviewboard.mozilla.org/r/225728/#review231730

> This was used in the last test as well - can this be pulled out into a helper function?

sure

> Was this line meant to be removed?

yes
Comment on attachment 8956105 [details]
Bug 1307928 - Add mocha tests for private messages; .

https://reviewboard.mozilla.org/r/225046/#review232196

> I'm not sure if having a separate function for this is worth the boilerplate vs checking `getAllMessagesUiById` alongside `getAllMessagesById` and `getVisibleMessages` in the function above. Up to you.

At first it was in the same function, but I felt like it was doing too much. I like that we focus on a single property here, with only the messages that we care about for the specific case.

> Similarly, I think this test case covers `"does group private messages"` assertions (and then some) - up to you if you want to keep that one or delete it.

I think you are right, we can remove the 2 "repeats" tests

> There's a lot of boilerplate to do this. I noticed we have do this in a few other places as well: https://searchfox.org/mozilla-central/search?q=const+%7B+dispatch%2C+getState+%7D+%3D+setupStore%28%5B%5D%2C+%7B&path=
> 
> Doesn't have to be done here, but worth considering / following up if we can make our setupStore helper function better support these use cases.

Do you mean, the cases where we explicitely define a hud to track releaseActors ?
Comment on attachment 8956104 [details]
Bug 1307928 - Remove private messages on lastPrivateContextExited event; .

https://reviewboard.mozilla.org/r/225044/#review231726

> I wonder if this condition should be handled inside the `removeMessagesFromState` function as an immediate return if there isn't anything to remove. There would be one extra clone of `...state` but we wouldn't have to worry about adding this extra check for other callers. What do you think?

I think it's important not returning a new object if there's nothing to do. If we do return a new instance, redux shallow comparison will be falsy and it will trigger a new rendering cycle (even if it won't go far because of shouldComponentUpdate in the ConsoleOutput).

I guess adding the check in removeMessagesFromState won't hurt and could help other callers.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31fce33230d4
Add a "private" property to the ConsoleMessage type; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/e5f12661cc1a
Remove private messages on lastPrivateContextExited event; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/991e19165a11
Add mocha tests for private messages; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/952534254538
Add mocha tests for NewConsoleOutputWrapper queues; r=bgrins.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: