Closed
Bug 1211665
Opened 9 years ago
Closed 8 years ago
Log messages for background scripts should appear in extension debugger
Categories
(WebExtensions :: Developer Tools, defect, P1)
WebExtensions
Developer Tools
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: nico.schloemer, Assigned: rpl)
References
(Blocks 1 open bug)
Details
(Whiteboard: triaged)
Attachments
(2 files, 1 obsolete file)
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.
Blocks: webext
Priority: -- → P1
Summary: webextensions api: background script inspection → Log messages for background scripts should appear in extension debugger
Updated•9 years ago
|
Flags: blocking-webextensions+
Updated•9 years ago
|
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 2•9 years ago
|
||
- 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•9 years 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•9 years ago
|
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
Comment 4•9 years ago
|
||
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)
Updated•9 years ago
|
Flags: needinfo?(kmaglione+bmo)
Updated•9 years ago
|
Assignee: nobody → lgreco
Assignee | ||
Updated•9 years 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•9 years 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•9 years 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 7•9 years ago
|
||
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+
Comment 8•9 years ago
|
||
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•9 years 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•9 years 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•9 years ago
|
Attachment #8727930 -
Flags: review+ → feedback?(amarchesini)
Comment 11•9 years ago
|
||
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•9 years 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•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42239/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42239/
Assignee | ||
Updated•9 years ago
|
Attachment #8727930 -
Flags: review?(amarchesini) → feedback?(amarchesini)
Assignee | ||
Comment 14•9 years 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)
Comment 15•9 years ago
|
||
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•9 years 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•9 years 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•9 years 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•9 years 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
Updated•9 years ago
|
Attachment #8727930 -
Flags: review?(amarchesini)
Comment 20•9 years ago
|
||
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•9 years 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•9 years 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•9 years 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•9 years 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•9 years 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•9 years 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•9 years 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•9 years ago
|
Whiteboard: triaged
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 28•9 years ago
|
||
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•9 years 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•9 years 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•9 years 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•9 years 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•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8737200 -
Flags: review?(amarchesini) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 34•9 years 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•9 years ago
|
||
bugherder |
Assignee | ||
Comment 37•9 years 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•9 years ago
|
Attachment #8734430 -
Flags: feedback?(poirot.alex)
Comment 38•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8734430 -
Flags: feedback?(poirot.alex) → feedback+
Assignee | ||
Comment 39•9 years 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 40•9 years ago
|
||
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•9 years 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•9 years 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•9 years 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)
Assignee | ||
Comment 44•9 years 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).
Comment 45•9 years ago
|
||
(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•9 years 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)
Comment 47•8 years ago
|
||
(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•8 years 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•8 years 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•8 years ago
|
||
checkin-needed added for Attachment 8734430 [details] (patch attached to this issue through mozreview)
Keywords: checkin-needed
Comment 51•8 years ago
|
||
Keywords: checkin-needed
Comment 52•8 years ago
|
||
bugherder |
Is this now complete, or is there more to land?
Flags: needinfo?(lgreco)
Assignee | ||
Comment 54•8 years 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•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: Developer tools
Flags: blocking-webextensions+
Assignee | ||
Updated•8 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•