Log messages for background scripts should appear in extension debugger

RESOLVED FIXED

Status

()

Toolkit
WebExtensions: Developer Tools
P1
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: nico.schloemer, Assigned: rpl)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Blocks: 1214433
Priority: -- → P1
Summary: webextensions api: background script inspection → Log messages for background scripts should appear in extension debugger

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1006102
(Assignee)

Comment 2

a year ago
Created attachment 8727930 [details]
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/
Attachment #8727930 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

a year ago
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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
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.
Attachment #8727930 - Flags: review?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)

Updated

a year ago
Assignee: nobody → lgreco
(Assignee)

Updated

a year ago
Attachment #8727930 - Attachment description: MozReview Request: Bug 1211665 - [webext] Console.jsm for webextension pages and contentWindow.console for content-scripts. r?kmag → MozReview Request: Bug 1211665 - ConsoleAPIStorage sets console message event's consoleID for WebExtension pages.
Attachment #8727930 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

a year ago
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/
(Assignee)

Comment 6

a year ago
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).
Attachment #8727930 - Flags: review?(kmaglione+bmo) → feedback?(amarchesini)
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
Attachment #8727930 - Flags: review+
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.
(Assignee)

Comment 9

a year ago
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/
Attachment #8727930 - Attachment description: MozReview Request: Bug 1211665 - ConsoleAPIStorage sets console message event's consoleID for WebExtension pages. → MozReview Request: Bug 1211665 - Log messages generated from WebExtensions pages should contains a ConsoleID. r?baku
Attachment #8727930 - Flags: feedback?(amarchesini)
(Assignee)

Comment 10

a year ago
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/
(Assignee)

Updated

a year ago
Attachment #8727930 - Flags: review+ → feedback?(amarchesini)
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.
Attachment #8727930 - Flags: feedback?(amarchesini)
(Assignee)

Comment 12

a year ago
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/
Attachment #8727930 - Flags: review?(amarchesini)
(Assignee)

Comment 13

a year ago
Created attachment 8734430 [details]
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/
(Assignee)

Updated

a year ago
Attachment #8727930 - Flags: review?(amarchesini) → feedback?(amarchesini)
(Assignee)

Comment 14

a year ago
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)
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
(Assignee)

Comment 16

a year ago
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/
Attachment #8727930 - Attachment description: MozReview Request: Bug 1211665 - Log messages generated from WebExtensions pages should contains a ConsoleID. r?baku → MozReview Request: Bug 1211665 - Save originAttributes in the console event messages. r?baku
Attachment #8727930 - Flags: feedback?(amarchesini) → review?(amarchesini)
(Assignee)

Comment 17

a year ago
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/
(Assignee)

Comment 18

a year ago
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.
(Assignee)

Comment 19

a year ago
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
Attachment #8727930 - Flags: review?(amarchesini)
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'.
(Assignee)

Comment 21

a year ago
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/
Attachment #8727930 - Flags: review?(amarchesini)
(Assignee)

Comment 22

a year ago
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/
(Assignee)

Comment 23

a year ago
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;
}
```
(Assignee)

Comment 24

a year ago
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/
(Assignee)

Comment 25

a year ago
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/
(Assignee)

Comment 26

a year ago
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)
(Assignee)

Comment 27

a year ago
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"

Updated

a year ago
Whiteboard: triaged
(Assignee)

Updated

a year ago
Keywords: leave-open
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()
Attachment #8727930 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 29

a year ago
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/
(Assignee)

Comment 30

a year ago
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/
(Assignee)

Comment 31

a year ago
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.
(Assignee)

Comment 32

a year ago
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().
(Assignee)

Comment 33

a year ago
Created attachment 8737200 [details] [diff] [review]
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
Attachment #8727930 - Attachment is obsolete: true
Attachment #8737200 - Flags: review?(amarchesini)
Attachment #8737200 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 34

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c3d66a71e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/75d0e51772db
Keywords: checkin-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
Flags: needinfo?(lgreco)

Comment 36

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44c3d66a71e3
(Assignee)

Comment 37

a year ago
(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.
Flags: needinfo?(lgreco)
(Assignee)

Updated

a year ago
Attachment #8734430 - Flags: feedback?(poirot.alex)
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)
Attachment #8734430 - Flags: feedback?(poirot.alex) → feedback+
(Assignee)

Comment 39

a year ago
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/
Attachment #8734430 - Attachment description: MozReview Request: Bug 1211665 - Filter add-ons console messages based on ConsoleID or originAttributes.addonId. → MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r?ochameau
Attachment #8734430 - Flags: feedback+ → review?(poirot.alex)
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.
Attachment #8734430 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 41

a year ago
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/
(Assignee)

Comment 42

a year ago
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
(Assignee)

Comment 43

a year ago
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?
Flags: needinfo?(poirot.alex)
Depends on: 1264106
(Assignee)

Comment 44

a year ago
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).
(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.
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 46

a year ago
(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?
Flags: needinfo?(poirot.alex)
(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.
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 48

a year ago
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/
Attachment #8734430 - Attachment description: MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r?ochameau → MozReview Request: Bug 1211665 - Filter add-ons console messages based on addonId. r=ochameau
(Assignee)

Comment 49

a year ago
(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.
(Assignee)

Comment 50

a year ago
checkin-needed added for Attachment 8734430 [details] (patch attached to this issue through mozreview)
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Blocks: 899054

Comment 51

a year ago
https://hg.mozilla.org/integration/fx-team/rev/3e8852edb5a0
Keywords: checkin-needed

Comment 52

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e8852edb5a0
Is this now complete, or is there more to land?
Flags: needinfo?(lgreco)
(Assignee)

Comment 54

a year ago
(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.
Flags: needinfo?(lgreco)
(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.

Updated

9 months ago
Component: WebExtensions: Untriaged → WebExtensions: Developer tools
Flags: blocking-webextensions+
(Assignee)

Updated

7 months ago
See Also: → bug 1318006
(Assignee)

Updated

7 months ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago7 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.