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.

- 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/

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

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 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 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 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 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/

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

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

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 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/

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.

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 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 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 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/

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 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 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/

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)

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 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 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 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/

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.

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

export of the first patch (as previously reviewed on mozreview): - Bug 1211665 - Save originAttributes in the console event messages. r=baku

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

(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

(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.

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 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 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 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/

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

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?

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.

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

(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 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/

(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.

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

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

(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.

(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.