Closed Bug 1442778 Opened 2 years ago Closed 7 months ago

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

Categories

(DevTools :: Console, enhancement, P2)

enhancement

Tracking

(firefox60 wontfix, firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox60 --- wontfix
firefox68 --- fixed

People

(Reporter: bgrins, Assigned: edenchuang)

References

Details

(Whiteboard: DWS_NEXT)

Attachments

(1 file, 7 obsolete files)

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.
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.
Attachment #8955914 - Flags: review?(bugs)
Comment on attachment 8955914 [details] [diff] [review]
part 2 - nsIConsoleMessage with isChromeContext

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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
Attachment #8955914 - Flags: review?(bugs) → review+
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)
Attachment #8957103 - Flags: review?(bugs) → review-
Attachment #8957103 - Attachment is obsolete: true
Attachment #8958085 - Flags: review?(bugs)
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.
Attachment #8958085 - Flags: review?(bugs) → review-
(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
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.
Flags: needinfo?(mdaly)
Andrew all yours.
Flags: needinfo?(mdaly) → needinfo?(bugmail)
Andrew, if you can take this bug would be great. It's important to have this flag for devtools. Thanks!
: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.
Assignee: amarchesini → bugmail
Flags: needinfo?(bugmail) → needinfo?(mdaly)
Flags: needinfo?(mdaly)
Priority: -- → P1
Product: Firefox → DevTools
Assignee: bugmail → nobody
Status: ASSIGNED → NEW
Priority: P1 → --
Assignee: nobody → bugmail
Priority: -- → P1
Priority: P1 → P2
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!
Flags: needinfo?(bugmail)
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.
Assignee: bugmail → nobody
Flags: needinfo?(bugmail)
Whiteboard: DWS_NEXT
(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 :)
Andrew, do you have updates on this?
Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)

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.

Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)
Assignee: nobody → echuang

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.

Attachment #8955915 - Attachment is obsolete: true
Attachment #9050330 - Flags: feedback?(amarchesini)
Attachment #8955914 - Attachment is obsolete: true
Attachment #9050331 - Flags: feedback?(amarchesini)

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.

Attachment #8958085 - Attachment is obsolete: true
Attachment #9050335 - Flags: feedback?(amarchesini)
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.
Attachment #9050330 - Flags: feedback?(amarchesini) → feedback+
Attachment #9050331 - Flags: feedback?(amarchesini) → feedback+
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.
Attachment #9050335 - Flags: feedback?(amarchesini) → feedback+
  1. Adding a new attribute chromeContext in ConsoleEvent
  2. Adding a new boolean attribute isFromChromeContext in nsIConsoleMessage
  3. Sending IsFromChromeContext to the parent process
Attachment #9050330 - Attachment is obsolete: true
Attachment #9050331 - Attachment is obsolete: true
Attachment #9050335 - Attachment is obsolete: true
Keywords: checkin-needed

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', '')

Flags: needinfo?(echuang)
Keywords: checkin-needed
Flags: needinfo?(echuang)
Keywords: checkin-needed

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fa7d6e6dedc
Add "chromeContext" parameter to console API and console service messages. r=smaug

Keywords: checkin-needed

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

Flags: needinfo?(echuang)

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

Flags: needinfo?(echuang)
Keywords: checkin-needed

Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/642420401dbe
Add "chromeContext" parameter to console API and console service messages. r=smaug

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

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.

Flags: needinfo?(echuang)
Regressions: 1546306
You need to log in before you can comment on or make changes to this bug.