1442778 - Add `chromeContext` parameter to console api and console service messages

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Brian Grinstead [:bgrins]]

This is for the platform portion of Bug 1260877 so we can provide filtering in the Browser Console UI, as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1260877#c16.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsIConsoleMessage with isChromeContext

The goal of this patch is to know if a nsIConsoleMessage (or better, a nsIScriptError) has been generated by a chrome context. I introduced a similar flag in ConsoleEvent in a separate patch, same bug.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - chromeContext in ConsoleEvent

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8955914 [details] [diff] [review]
part 2 - nsIConsoleMessage with isChromeContext

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  e1db891b3ad348ae7d039255e022c4f64e1c532c
>
>diff --git a/dom/bindings/nsIScriptError.idl b/dom/bindings/nsIScriptError.idl
>--- a/dom/bindings/nsIScriptError.idl
>+++ b/dom/bindings/nsIScriptError.idl
>@@ -74,16 +74,18 @@ interface nsIScriptError : nsIConsoleMes
>     readonly attribute unsigned long long outerWindowID;
> 
>     /* Get the inner window id this was initialized with.  Zero will be
>        returned if init() was used instead of initWithWindowID(). */
>     readonly attribute unsigned long long innerWindowID;
> 
>     readonly attribute boolean isFromPrivateWindow;
> 
>+    readonly attribute boolean isChromeContext;
The error is not chrome context, but perhaps follow the naming for isFromPrivateWindow -
so, isFromChromeWindow

>+       mIsFromPrivateWindow(false),
>+       mIsChromeContext(false)
same here

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - sending isFromChromeContext to the parent process

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8957103 [details] [diff] [review]
part 3 - sending isFromChromeContext to the parent process

>@@ -4152,17 +4152,18 @@ nsContentUtils::LogSimpleConsoleError(co
>     do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
>   if (scriptError) {
>     nsCOMPtr<nsIConsoleService> console =
>       do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>     if (console && NS_SUCCEEDED(scriptError->Init(aErrorText, EmptyString(),
>                                                   EmptyString(), 0, 0,
>                                                   nsIScriptError::errorFlag,
>                                                   classification,
>-                                                  false /* from private window */))) {
>+                                                  false /* from private window */,
>+                                                  false /* from chrome context */))) {
Why false? This sure can be called on chrome context too

>+++ b/dom/security/nsCSPUtils.cpp
>@@ -165,17 +165,18 @@ CSP_LogMessage(const nsAString& aMessage
>                                  aSourceLine, aLineNumber,
>                                  aColumnNumber, aFlags,
>                                  catStr, aInnerWindowID);
>   }
>   else {
>     rv = error->Init(cspMsg, aSourceName,
>                      aSourceLine, aLineNumber,
>                      aColumnNumber, aFlags,
>-                     aCategory, false /* from private window */);
>+                     aCategory, false /* from private window */,
>+                     true /* from chrome context */);
why true


>+++ b/dom/websocket/WebSocket.cpp
>@@ -386,17 +386,18 @@ WebSocketImpl::PrintErrorOnConsole(const
>                                        mScriptColumn,
>                                        nsIScriptError::errorFlag, "Web Socket",
>                                        mInnerWindowID);
>   } else {
>     rv = errorObject->Init(message,
>                            NS_ConvertUTF8toUTF16(mScriptFile),
>                            EmptyString(), mScriptLine, mScriptColumn,
>                            nsIScriptError::errorFlag, "Web Socket",
>-                           false /* from private window */);
>+                           false /* from private window */,
>+                           true /* from chrome context */);
why true?

(perhaps those 'true' cases are ok, but please explain. The false case isn't ok)

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 3 - sending isFromChromeContext to the parent process

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8958085 [details] [diff] [review]
part 3 - sending isFromChromeContext to the parent process

It isn't quite clear to me why ReportInternalError uses chrome-only, but I guess that is fine.

Why nsCORSListenerProxy.cpp uses chrome context? Chrome context but possibly private browsing flag set - feel odd.
It is possible that I don't quite understand what we want the new flag to tell.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8958085 [details] [diff] [review]
> part 3 - sending isFromChromeContext to the parent process
> 
> It isn't quite clear to me why ReportInternalError uses chrome-only, but I
> guess that is fine.
> 
> Why nsCORSListenerProxy.cpp uses chrome context? Chrome context but possibly
> private browsing flag set - feel odd.
> It is possible that I don't quite understand what we want the new flag to
> tell.

The new flag is so that we can implement a "Show content messages" checkbox in the Browser Console as a way to filter out a bunch of the noise generated by web pages that Firefox devs don't want to see.

It may also be useful for the browser error reporting into sentry which currently looks at the message category to decide if it was from chrome: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/modules/BrowserErrorReporter.jsm#135

Comment 9

[Andrea Marchesini [:baku]]

Marion, I'm not able to work on this at the moment but it's important to land it.
The code is almost ready. It's just matter of apply comments and replay to comment 7.

Comment 10

[Marion Daly [:mdaly]]

Andrew all yours.

Comment 11

[Andrea Marchesini [:baku]]

Andrew, if you can take this bug would be great. It's important to have this flag for devtools. Thanks!

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

:mdaly, can you assign a priority?  Unsure if this is P1 or P2, but from dev-platform it does sound like it's blocking forward movement on browser console changes or at least usefulness.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

With Fission coming in, this might help mitigate the risk of performance issues in the Browser Console, which is very important for us. Furthermore, it's blocking Bug 1260877 which we really want to ship in Q4.

Andrew, are you working on this, or planning to do so in the next weeks? That would be really rad, thanks!

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Apologies for letting this languish in secret backlog; moving this to our DWS_NEXT queue where it should be picked up in the next week or two by a team-member with a few more cycles.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Andrew Sutherland [:asuth] from comment #14)
> Apologies for letting this languish in secret backlog; moving this to our
> DWS_NEXT queue where it should be picked up in the next week or two by a
> team-member with a few more cycles.

No worries, thanks for replying quickly :)

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Andrew, do you have updates on this?

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Caught up with :Honza on slack, the current expected schedule is near the end of Q1 landing in 68 shortly after that branch opens on central. If :baku has cycles, that would also be great, however.

Comment 18

[Eden Chuang[:edenchuang]]

Attachment: part 1 - chromeContext in ConsoleEvent

Hello Baku. I am continuing your work.
I just rebased the patches.
Before I request a review to Olli, I would like to get some feedback from you first.

Comment 19

[Eden Chuang[:edenchuang]]

Attachment: part 2 - nsIConsoleMessage with isChromeContext

Comment 20

[Eden Chuang[:edenchuang]]

Attachment: part 3 - sending isFromChromeContext to the parent process

I am not sure if the behavior is correct or not. Could you give me some feedback. Especially on followings

1. PostMessageEvent creation.
2. netwerk/protocol/http/HttpChannelChild.cpp and netwerk/protocol/http/nsCORSListenerProxy

Thanks.

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 9050330 [details] [diff] [review]
part 1 - chromeContext in ConsoleEvent

Review of attachment 9050330 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/Console.webidl
@@ +107,4 @@
>    any timer = null;
>    any counter = null;
>    DOMString prefix = "";
> +  boolean chromeContext = false; 

extra space here.

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 9050335 [details] [diff] [review]
part 3 - sending isFromChromeContext to the parent process

Review of attachment 9050335 [details] [diff] [review]:
-----------------------------------------------------------------

In general, it looks good. I think the using of loadingPrincipal is wrong. PostMessageEvent can probably check if the providedPrincipal is system.

::: dom/base/PostMessageEvent.h
@@ +40,4 @@
>                     nsIURI* aCallerDocumentURI)
>        : PostMessageEvent(aSource, aCallerOrigin, aTargetWindow,
>                           aProvidedPrincipal, Some(aCallerWindowID),
> +                         aCallerDocumentURI, false, true) {}

This seems wrong. Why this is true? Probably you can use the provided principal to determine if this is chrome or non-chrome context.

@@ +46,5 @@
>    // example because it lives in a different process).
>    PostMessageEvent(BrowsingContext* aSource, const nsAString& aCallerOrigin,
>                     nsGlobalWindowOuter* aTargetWindow,
>                     nsIPrincipal* aProvidedPrincipal, nsIURI* aCallerDocumentURI,
> +                   bool aIsFromPrivateWindow, bool aIsFromChromeContext)

You can do the same here.

::: dom/indexedDB/ScriptErrorHelper.cpp
@@ +111,5 @@
>            aMessage, aFilename,
>            /* aSourceLine */ EmptyString(), aLineNumber, aColumnNumber,
>            aSeverityFlag, category.get(),
> +          /* IDB doesn't run on Private browsing mode */ false,
> +          /* from chrome context */ true));

Always?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +3859,5 @@
>      bool privateBrowsing =
>          !!mLoadInfo->GetOriginAttributes().mPrivateBrowsingId;
> +    nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +    mLoadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal)); 
> +    bool fromChromeContext = nsContentUtils::IsSystemPrincipal(loadingPrincipal);

See the next comments.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +103,5 @@
>      privateBrowsing = nsContentUtils::IsInPrivateBrowsing(loadGroup);
>    }
>  
> +  bool fromChromeContext = true;
> +  nsCOMPtr<nsILoadInfo> loadInfo;

nsCOMPtr<nsILoadInfo> loadInfo = channel->LoadInfo();

@@ +106,5 @@
> +  bool fromChromeContext = true;
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  channel->GetLoadInfo(getter_AddRefs(loadInfo));
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +  loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal)); 

check the return value here.

@@ +107,5 @@
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  channel->GetLoadInfo(getter_AddRefs(loadInfo));
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +  loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal)); 
> +  fromChromeContext = nsContentUtils::IsSystemPrincipal(loadingPrincipal);

This seems wrong. Why do you use the loading principal? If this is the top-level resource, the loadingPrincipal is system. Probably you want to get the channel result principal. See: nsIScriptSecurityManager::GetChannelResultPrincipal()

@@ +1566,5 @@
>                             0,              // lineNumber
>                             0,              // columnNumber
>                             nsIScriptError::warningFlag, category.get(),
> +                           aPrivateBrowsing,
> +                           true);          // From chrome context

You should use aFromChromeContext, I guess.

Comment 23

[Eden Chuang[:edenchuang]]

Attachment: Add "chromeContext" parameter to console API and console service messages.

1. Adding a new attribute chromeContext in ConsoleEvent
2. Adding a new boolean attribute isFromChromeContext in nsIConsoleMessage
3. Sending IsFromChromeContext to the parent process

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please rebase the patch, it fails with:
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp13i3Ze\npatching file netwerk/protocol/http/nsCORSListenerProxy.cpp\nHunk #3 FAILED at 608\nHunk #4 FAILED at 640\nHunk #5 FAILED at 1237\n3 out of 7 hunks FAILED -- saving rejects to file netwerk/protocol/http/nsCORSListenerProxy.cpp.rej\npatching file netwerk/protocol/http/HttpChannelChild.cpp\nHunk #1 FAILED at 616\nHunk #2 FAILED at 626\nHunk #3 FAILED at 965\n3 out of 4 hunks FAILED -- saving rejects to file netwerk/protocol/http/HttpChannelChild.cpp.rej\nabort: patch failed to apply', '')

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fa7d6e6dedc
Add "chromeContext" parameter to console API and console service messages. r=smaug

Comment 26

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-mochitest-devtools-chrome-e10s-1%2Cm-e10s%28dt1%29&fromchange=7fa7d6e6dedc32fbc59e3dcae4c58cc38bb86921&tochange=a63e42e477b208cabba83030b4ebb5d31111317e&selectedJob=236162247

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236161828&repo=autoland&lineNumber=5467

Backout link: https://hg.mozilla.org/integration/autoland/rev/a63e42e477b208cabba83030b4ebb5d31111317e

[task 2019-03-26T17:55:21.693Z] 17:55:21 INFO - TEST-PASS | devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js | wrapper exists -
[task 2019-03-26T17:55:21.693Z] 17:55:21 INFO - Buffered messages logged at 17:55:18
[task 2019-03-26T17:55:21.695Z] 17:55:21 INFO - Console message: [JavaScript Warning: "onmozfullscreenchange is deprecated." {file: "http://example.com/browser/devtools/client/webconsole/test/fixtures/stub-generators/test-console-api.html" line: 0}]
[task 2019-03-26T17:55:21.696Z] 17:55:21 INFO - Console message: [JavaScript Warning: "onmozfullscreenerror is deprecated." {file: "http://example.com/browser/devtools/client/webconsole/test/fixtures/stub-generators/test-console-api.html" line: 0}]
[task 2019-03-26T17:55:21.696Z] 17:55:21 INFO - Buffered messages logged at 17:55:21
[task 2019-03-26T17:55:21.696Z] 17:55:21 INFO - Removing tab.
[task 2019-03-26T17:55:21.696Z] 17:55:21 INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2019-03-26T17:55:21.696Z] 17:55:21 INFO - Got event: 'TabClose' on [object XULElement].
[task 2019-03-26T17:55:21.696Z] 17:55:21 INFO - Tab removed and finished closing
[task 2019-03-26T17:55:21.699Z] 17:55:21 INFO - Buffered messages finished
[task 2019-03-26T17:55:21.699Z] 17:55:21 INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js | The consoleApi stubs file needs to be updated by running mach test devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_update_stubs_console_api.js -
[task 2019-03-26T17:55:21.699Z] 17:55:21 INFO - Stack trace:
[task 2019-03-26T17:55:21.700Z] 17:55:21 INFO - chrome://mochikit/content/browser-test.js:test_ok:1304
[task 2019-03-26T17:55:21.700Z] 17:55:21 INFO - chrome://mochitests/content/browser/devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js:null:17
[task 2019-03-26T17:55:21.700Z] 17:55:21 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
[task 2019-03-26T17:55:21.700Z] 17:55:21 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1134
[task 2019-03-26T17:55:21.700Z] 17:55:21 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:995
[task 2019-03-26T17:55:21.701Z] 17:55:21 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-03-26T17:55:21.716Z] 17:55:21 INFO - Leaving test bound
[task 2019-03-26T17:55:21.716Z] 17:55:21 INFO - GECKO(2537) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2019-03-26T17:55:21.716Z] 17:55:21 INFO - GECKO(2537) | MEMORY STAT heapAllocated not supported in this build configuration.
[task 2019-03-26T17:55:21.717Z] 17:55:21 INFO - GECKO(2537) | MEMORY STAT | vsize 20973942MB | residentFast 1114MB
[task 2019-03-26T17:55:21.718Z] 17:55:21 INFO - TEST-OK | devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_console_api.js | took 8454ms

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Eden, first, thank you very much for working on this!

In the console we have mocha tests that relies on what we call stubs: js object that represent the packets sent from the server to the client, for different kind of messages (console.* calls, network messages, …).

Here since your patch impacts the shape of console messages, these stubs need to be updated.

You can do that by running mach test devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_update_stubs_console_api.js (I'd suggest to run every browser_webconsole_update_stubs_* tests).
You can then check that everything is okay by running ``mach test devtools/client/webconsole/test/fixtures/stub-generators/(it will run all thecheck_stubs*` tests).

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/642420401dbe
Add "chromeContext" parameter to console API and console service messages. r=smaug

Comment 29

[Noemi Erli[:noemi_erli]]

Backed out changeset 6ccad746f5d8 (Bug 1456569) for Linting opt failure in TypedArray.js CLOSED TREE

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=642420401dbe4795fd32d7c3de41d2dad5d733bc&selectedJob=237483600

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=237483600&repo=autoland&lineNumber=281

Backout: https://hg.mozilla.org/integration/autoland/rev/68ac00e863e05091280bf23765607af045cba3cc

Comment 30

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/642420401dbe

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please ignore comment 29, that was intended for a different bug - bug 1456569. Bug 1442778 hasn't been backed out and has been merged to mozilla-central.