1211665 - Log messages for background scripts should appear in extension debugger

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, defect, P1)

Description

[nico.schloemer]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

I'm using the new WebExtensions API with a background script. To debug, I'd like to have console in the context of the background script, i.e., a console to which all log messages are posted.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

- create a console object using the Console jsm for webextensions, which generates console message events
  which can be already been collected by the Addon Debugger's webconsole
- use the contentWindow console object for content-scripts, which generates console message events
  which can be already logged in the Tab's webconsole

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

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

I'd like to re-open this issue to split the above proposed short-run fix (restricted to the issue of the console messages) from the issue of collecting the Error messages.

The goal is to:

- log all the console messages generated by the WebExtensions pages (e.g. background pages, popup pages, tab pages) in the Adddon Debugger, by "temporarily" injecting the Console jsm into the WebExtension contexts (where "temporarily" means: until the Addon Debugger explictly introduces support for WebExtensions add-ons, eg. by checking that the window associated to a log message is one of the window objects associated to the debugged WebExtensions)

- log all the console messages generated by a WebExtension content-scripts in the Tab's webconsole, by copying the console object from the wrapped content window to the content-scripts' sandbox.

without making any changes to the current Addon Debugger.

On the long run, a more complete solution would be a different Addon Debugger for WebExtensions which works much more like the Tab / Browser toolbox, e.g. it could collect the console messages and the logged errors based on the window which has originated them (by checking if it is originated from a contentWindow related to one of the tracked WebExtension contexts) and it would be able to support the other Toolbox panels (like the DOM inspector, the button to change the current selected frame etc., which didn't make sense, or that were particularly challenging, for the other add-on technologies), and the above workaround will not be needed anymore.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

https://reviewboard.mozilla.org/r/38691/#review35525

Per our discussion on IRC, I'm worried about injecting a ConsoleAPI instance into a non-privileged scope. I think we have safer options.

::: toolkit/components/extensions/Extension.jsm:466
(Diff revision 1)
> +          consoleID: extension.id ? "addon/" + extension.id : "",

`extension.id` should always be set here.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

This patch takes a different approach to set the consoleID on the console API events generated by a WebExtension ExtensionPage (the consoleID is used by the Addon Debugger to filter out any console api event which is not related to the addon targeted by the Addon Debugger):

In the ConsoleAPIStorage.js, the window which has generated the event is retrieved using its id and then (if the window has been found) the addonId is extracted from the "document.nodePrincipal.originAttributes", and the consoleID of the event is set to the value expected ("addon/ADDONID") before caching it and sending it the event around, by notifying the console events observers (and so before it has been received by the Addon Debugger's remote debugging actors).

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

https://reviewboard.mozilla.org/r/38691/#review36995

It's ok. f+ but please test that content loaded by an addon doesn't have originAttributes.addonId set.

::: dom/base/ConsoleAPIStorage.js:134
(Diff revision 2)
> +    // Check if the window is from an Addon (e.g. a WebExtension page) and
> +    // if it is then extract the addonId and save it as the "consoleID"
> +    // attribute of the event.
> +    let msgWindow = Services.wm.getCurrentInnerWindowWithId(aId);
> +    if (msgWindow) {
> +      let {originAttributes: {addonId}} = msgWindow.document.nodePrincipal;

you need just the addonId, then:

let addonId = msgWindow.document.nodePrincipal.originAttributes.addonId;

::: dom/tests/browser/browser.ini:26
(Diff revision 2)
>  [browser_ConsoleStorageAPITests.js]
>  skip-if = e10s
>  [browser_ConsoleStoragePBTest_perwindowpb.js]
>  skip-if = e10s
> +[browser_ConsoleStorageAddonConsoleID.js]
> +skip-if = e10s

I know that you are following the same pattern of the other tests, but why do we skip it for e10s?

::: dom/tests/browser/browser_ConsoleStorageAddonConsoleID.js:51
(Diff revision 2)
> +
> +  let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +  let uuid = uuidGenerator.generateUUID().number;
> +  uuid = uuid.slice(1, -1); // Strip { and } off the UUID.
> +  let baseURI = NetUtil.newURI("moz-extensions://${uuid}");
> +  let originAttributed = {addonId: FAKE_ADDON_ID};

originAttributes

Comment 8

[Andrea Marchesini [:baku]]

Question: what generates this Console Events? Console API or Console.jsm?
What about if it's the Console API to set the correct IDs using the principal that has been used by the content?
This seems to me the correct approach and it will work also if these ConsoleEvents are executed by Workers/SharedWorkers/etc.
I'm saying this because with your patch, if this ConsoleEvent comes from a SharedWorker/ServiceWorker you are not able to set the correct ID because the window will be null.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

After discussing this patch on IRC, I and lgreco decided that is better to store the OriginAttributes into the ConsoleEvent instead adding an additional ID.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Changed from review to feedback because, even if the new test case passes after the last changes, and I tested it in a real addon, I'm still figuring out "how many of the other test cases these patches can brake" (and so I am waiting to look at the results of a more broad try build and then re-look at the changes applied by this patch)

Comment 15

[Andrea Marchesini [:baku]]

https://reviewboard.mozilla.org/r/38691/#review39459

::: dom/base/Console.h:313
(Diff revision 5)
>    PRThread* mOwningThread;
>  #endif
>  
>    uint64_t mOuterID;
>    uint64_t mInnerID;
> +  JS::Value mOriginAttributes;

This should be OriginAttributes and not JS::Value

::: dom/base/Console.cpp:143
(Diff revision 5)
>      mInnerIDString = aInnerID;
>      mIDType = eString;
>    }
>  
>    void
> +  SetOriginAttributes(const JS::Value& aOriginAttributes)

Use the OriginAttributes. See caps/BasePrincipal.h

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/38691/#review39459

> This should be OriginAttributes and not JS::Value

I opted for removing this from the Console.h, the last patch revision adds the OriginAttributes only in the ConsoleCallData.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

In the last revision of this patch, the OriginAttributes are copied from the document principal into the ConsoleCallData, and then eventually converted into a JS::Value accessible from JavaScript once the message is prepared to be sent to the console listener in the Console::ProcessCallData method.

Follows a couple of pushes to try:

- try build with all mochitests (only the first patch in the mozreview queue):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ef8edf6426

- try build with mochitest-dt and mochitest-e10s-devtools-chrome (both the patches):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0266e6eef54

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

https://reviewboard.mozilla.org/r/38691/#review39509

Good! Can I see it again before landing it?

::: dom/base/Console.cpp:565
(Diff revision 6)
>      AssertIsOnMainThread();
>      MOZ_ASSERT(mCallData->mCopiedArguments.IsEmpty());
>  
>      // The windows have to run in parallel.
>      MOZ_ASSERT(!!aOuterWindow == !!aInnerWindow);
> +    nsIPrincipal* principal;

1. nsCOMPtr<nsIPrincipal>
2. remove it from here.

::: dom/base/Console.cpp:572
(Diff revision 6)
>      if (aOuterWindow) {
> +      // Save the principal's OriginAttributes in the console event data
> +      // so that we will be able to filter messages by origin attributes.
> +      nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(aInnerWindow);
> +      if (sop) {
> +        principal = sop->GetPrincipal();

nsCOMPtr<nsIPrincipal> principal = ...

::: dom/base/Console.cpp:573
(Diff revision 6)
> +      // Save the principal's OriginAttributes in the console event data
> +      // so that we will be able to filter messages by origin attributes.
> +      nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(aInnerWindow);
> +      if (sop) {
> +        principal = sop->GetPrincipal();
> +        if (principal) {

Plus... if we don't have a principal, we don't want to continue.

if (NS_WARN_IF(!principal)) {
  return;
}

aCallData->SetOriginAttributes(...

::: dom/base/Console.cpp:601
(Diff revision 6)
>          innerID = NS_LITERAL_STRING("Worker");
>        }
>  
> +      // Save the principal's OriginAttributes in the console event data
> +      // so that we will be able to filter messages by origin attributes.
> +      principal = mWorkerPrivate->GetPrincipal();

nsCOMPtr<principal> principal = ...
if (NS_WARN_IF(!principal)) {
  return;
}

::: dom/base/Console.cpp:1294
(Diff revision 6)
>    }
>  
>    JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
>  
>    if (NS_IsMainThread()) {
> +    if (mWindow) {

This is interesting... if we are on the main-thread, we must have a window.

Can you file a bug to enforce this and CC/Assign to me? In the code seems that we can be here but mWindow can be null. This should not happen.

::: dom/base/Console.cpp:1420
(Diff revision 6)
>    event.mTimeStamp = aData->mTimeStamp;
>    event.mPrivate = aData->mPrivate;
>  
> +  // Save the principal's OriginAttributes in the console event data
> +  // so that we will be able to filter messages by origin attributes.
> +  JS::Rooted<JS::Value> mOriginVal(cx);

In gecko, we use mFoobar when this 'foobar' is a member of the class. aFoobar when it's a parameter of a function/method.

Here you should just call it 'originAttributesValue'.

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/38691/#review39509

> Plus... if we don't have a principal, we don't want to continue.
> 
> if (NS_WARN_IF(!principal)) {
>   return;
> }
> 
> aCallData->SetOriginAttributes(...

I've added this kind of "check + log & return if it fails" for both the "sop" and the "principal" pointers:

```
nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(mWindow);
if (NS_WARN_IF(!sop)) {
  return;
}

nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();
if (NS_WARN_IF(!principal)) {
  return;
}
```

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

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

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/38691/#review39509

> This is interesting... if we are on the main-thread, we must have a window.
> 
> Can you file a bug to enforce this and CC/Assign to me? In the code seems that we can be here but mWindow can be null. This should not happen.

I was going to report this as a separate issue, but I noticed a number of changes in the version of Console.cpp in the fx-team tip and I thought that it could be better
to rebase and resolve the conflicts before reviewing the last version of this patch.

After that, I opted to move this fragment into a "if (mWindow)" that was already in this method (and that is out of the "if on the main-thread" check), where it seems to be that it fits better, how it looks to you?.

(I'm not clearing this issue because is still under discussion)

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/38691/#review39509

> I was going to report this as a separate issue, but I noticed a number of changes in the version of Console.cpp in the fx-team tip and I thought that it could be better
> to rebase and resolve the conflicts before reviewing the last version of this patch.
> 
> After that, I opted to move this fragment into a "if (mWindow)" that was already in this method (and that is out of the "if on the main-thread" check), where it seems to be that it fits better, how it looks to you?.
> 
> (I'm not clearing this issue because is still under discussion)

Bugzilla issue opened as "Bug 1261115 - when Console is running in the main thread the existence of mWindow should always be ensured"

Comment 28

[Andrea Marchesini [:baku]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

https://reviewboard.mozilla.org/r/38691/#review40309

::: dom/base/Console.cpp:657
(Diff revision 8)
>          id.AssignWithConversion(mWorkerPrivate->WorkerName());
>        } else {
>          innerID = NS_LITERAL_STRING("Worker");
>        }
>  
> +      // Save the principal's OriginAttributes in the console event data

Move this block after SetIDs()

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8727930 [details]
MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38691/diff/8-9/

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/38689/#review40353

::: dom/base/Console.cpp:1278
(Diff revisions 8 - 9)
>                                         aData, this))) {
>      return;
>    }
>  
>    if (mWindow) {
>      nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(mWindow);

It seems that reviewboard renders this a remove, but the fragment is moved back here to its previous position.

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/38691/#review40309

> Move this block after SetIDs()

In the last revision of this patch I moved this block (and the other one in the same method) after the SetIDs().

Comment 33

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 1-Bug_1211665___Save_originAttributes_in_the_console_event_messages__r_baku.patch

export of the first patch (as previously reviewed on mozreview):

- Bug 1211665 - Save originAttributes in the console event messages. r=baku

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44c3d66a71e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/75d0e51772db

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Pulsebot from comment #34)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/44c3d66a71e3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/75d0e51772db

That second patch apparently wasn't supposed to land. Backed it out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ea6a3bb8a6

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/44c3d66a71e3

Comment 37

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Wes Kocher (:KWierso) from comment #35)
> (In reply to Pulsebot from comment #34)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/44c3d66a71e3
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/75d0e51772db
> 
> That second patch apparently wasn't supposed to land. Backed it out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ea6a3bb8a6

Confirmed, the second patch wasn't supposed to land yet.

Thanks again Wes for helping me to immediately remove the patch.

Comment 38

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/42239/#review41281

::: devtools/shared/webconsole/utils.js:926
(Diff revision 5)
> +      // the Console API object.
> +      if (message.originAttributes &&
> +          message.originAttributes.addonId !== this.addonId) {
> +        return false;
> +      }
> +    }

Given that the platform patch already landed,
we can assumer that origiAttributes will be set.
You should be able to remove all the compatibility code and only keep originAttributes check.

And also simplify ConsoleAPIListener. Keep the last argument as a dictionnary, it is great and may help us filter differently in future.

Then you can repurpose the following test against just { addonId } parameter:
devtools/shared/tests/unit/test_consoleID.js
(rename this file accordingly, like test_console_filtering or something)

Comment 39

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 40

[Alexandre Poirot [:ochameau]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

https://reviewboard.mozilla.org/r/42239/#review42283

Looks good, do you have a green try?

::: devtools/shared/tests/unit/test_console_filtering.js:4
(Diff revision 6)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm");

In /devtools/ we have a helper to get Services:
const Services = require("Services");

::: devtools/shared/tests/unit/test_console_filtering.js:35
(Diff revision 6)
>    }
>  };
>  
> +function createFakeAddonWindow({addonId} = {}) {
> +  let baseURI = Services.io.newURI("about:blank", null, null);
> +  let originAttributes = {addonId: "bar"};

Either use the functionargument, or drop it.

::: devtools/shared/webconsole/utils.js:907
(Diff revision 6)
> +      // used in Addon SDK add-ons), the standard 'console' object
> +      // (which is used in regular webpages and in WebExtensions pages)
> +      // contains the originAttributes of the source document principal.
> +
> +      // Filtering based on the old-style consoleID property used by
> +      // the legacy Console JSM module.

Oh I didn't realized that, nice catch.
I'm not sure there is enough value to make ConsoleAPI.jsm also set originAttributes?

::: devtools/shared/webconsole/utils.js:917
(Diff revision 6)
> +      // Filtering based on the originAttributes used by
> +      // the Console API object.
> +      if (message.originAttributes &&
> +          message.originAttributes.addonId == this.addonId) {
> +        return true;
> +      }

I would put that first, so that WebExtension codepath is very slightly faster than SDK.

Comment 41

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/42239/#review42283

pushed the last revision of this patch on try (and it is still running):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf72a0f3198

> Oh I didn't realized that, nice catch.
> I'm not sure there is enough value to make ConsoleAPI.jsm also set originAttributes?

I'm not against it, on the contrary it is something that I was already thinking of,
but I would prefer to evaluate and eventually do it in a followups, because it is something that needs to be changed in more than one places (and I'm sure if last of them are actually used from anyone), eg.:

- https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/console/plain-text.js#54
- https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#827
- https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4565
- https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/EmbedRT.js#41

Comment 43

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've just found during a search on dxr, that the webconsole removes the consoleID before sending the console event object to the RDP client:

>  prepareConsoleMessageForRemote:
>  function WCA_prepareConsoleMessageForRemote(aMessage, aUseObjectGlobal = true)
>  {
>    let result = WebConsoleUtils.cloneObject(aMessage);
>    ...
>    delete result.consoleID;

From https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js#1615

I didn't notice it until now, but I'm thinking that I should probably do the same for the originAttributes,
am I right?

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

Pushed on try again (this time rebased on mozilla-central), because the previous try push (based on fx-team) and it had failures (that at a first look seems unrelated) and I wanted to see how it goes with the patch rebased on mozilla-central:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b43c43135464&selectedJob=19383690

even if the push is still running, in the optimized builds the mochitests are already completed and green, but the debug builds contains a bunch of orange that I'm looking into (to be sure if they are or are not related to this change).

Comment 45

[Alexandre Poirot [:ochameau]]

(In reply to Luca Greco [:rpl] from comment #43)
> I've just found during a search on dxr, that the webconsole removes the
> consoleID before sending the console event object to the RDP client:
> 
> >  prepareConsoleMessageForRemote:
> >  function WCA_prepareConsoleMessageForRemote(aMessage, aUseObjectGlobal = true)
> >  {
> >    let result = WebConsoleUtils.cloneObject(aMessage);
> >    ...
> >    delete result.consoleID;
> 
> From
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> webconsole.js#1615
> 
> I didn't notice it until now, but I'm thinking that I should probably do the
> same for the originAttributes,
> am I right?

Yes. Do not hesitate to audit this code to see if we slips any other useless field here.

Comment 46

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Alexandre Poirot [:ochameau] from comment #40)
> Comment on attachment 8734430 [details]
> MozReview Request: Bug 1211665 - Filter add-ons console messages based on
> addonId. r?ochameau
> 
> https://reviewboard.mozilla.org/r/42239/#review42283
> 
> Looks good, do you have a green try?

None of the oranges in the above push to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=b43c43135464)
seems to be related to this change.

I prepared the other patch which remove the originAttributes from the result in the prepareConsoleMessageForRemote method (as described in Comment 43), but I'm still looking for the best place to test it.

Should we land the Attachment 8734430 [details] in the mean time?

Comment 47

[Alexandre Poirot [:ochameau]]

(In reply to Luca Greco [:rpl] from comment #46)
> Should we land the Attachment 8734430 [details] in the mean time?

Yes!! It isn't a big deal. Do not hesitate to spread your work in followups. It easier to review focused patches.

Comment 48

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8734430 [details]
MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau

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

Comment 49

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Luca Greco [:rpl] from comment #48)
> Comment on attachment 8734430 [details]
> MozReview Request: Bug 1211665 - Filter add-ons console messages based on
> addonId. r=ochameau
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/42239/diff/7-8/

Rebased Attachment 8734430 [details] on a recent fx-team tip before adding the checkin-needed.

Comment 50

[Luca Greco [:rpl] [:luca] [:lgreco]]

checkin-needed added for Attachment 8734430 [details] (patch attached to this issue through mozreview)

Comment 51

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e8852edb5a0

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e8852edb5a0

Comment 53

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is this now complete, or is there more to land?

Comment 54

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #53)
> Is this now complete, or is there more to land?

I'm waiting the landing of the patch attached on Bug 1264106.

Besides that, I've one more patch for the change described in Comment 43, Comment 46 and Comment 47.
I'm still working on it (on the testing part, I didn't found an existent test case for it, and so I'm probably going to create a new test file for it)

as it is not strictly related to the issue described here, I can create a follow-up issue which describe it and attach the patches on it.

Comment 55

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Luca Greco [:rpl] from comment #54)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #53)
> > Is this now complete, or is there more to land?
> 
> I'm waiting the landing of the patch attached on Bug 1264106.
> 
> Besides that, I've one more patch for the change described in Comment 43,
> Comment 46 and Comment 47.
> I'm still working on it (on the testing part, I didn't found an existent
> test case for it, and so I'm probably going to create a new test file for it)
> 
> as it is not strictly related to the issue described here, I can create a
> follow-up issue which describe it and attach the patches on it.

I am not sure about the details of this bug, but if landing the rest could bleed into the next release (less than a week from now), then a follow up bug seems like a good idea.