Closed
Bug 1347217
Opened 4 years ago
Closed 4 years ago
Big blobs of stuff dumped to stdout or stderr on startup
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bzbarsky, Assigned: keeler)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-release+
|
Details |
The console.log calls in browser/extensions/deployment-checker/bootstrap.js are dumping huge blobs of stuff out to stdout or stderr (I didn't check which). This is quite annoying, since it completely destroys terminal scrollback. It's not clear to me why we're doing this, especially in non-debug builds.
Flags: needinfo?(dkeeler)
![]() |
Assignee | |
Comment 1•4 years ago
|
||
Some debugging information will be helpful for QA to verify that this is working as intended when we deploy this as a go-faster add-on, but the deluge of output is definitely not helpful. I'll trim it down.
Assignee: nobody → dkeeler
Flags: needinfo?(dkeeler)
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment hidden (mozreview-request) |
![]() |
Reporter | |
Comment 3•4 years ago
|
||
We shouldn't be creating any stdout/stderr output at all in opt builds, generally. Logging stuff to the browser console maybe makes sense. Dumping it all out (maybe because the dump.enabled pref is set in my case?) really doesn't.
Comment 4•4 years ago
|
||
console.log should be going to the browser console by default, no? I don't know why that would be going to stdout/stderr by default.
![]() |
Reporter | |
Comment 5•4 years ago
|
||
> I don't know why that would be going to stdout/stderr by default.
It generally doesn't. But in this case it is, and the question is why.
Comment 6•4 years ago
|
||
mozreview-review |
Comment on attachment 8847187 [details] bug 1347217 - lessen debug spew in deployment-checker https://reviewboard.mozilla.org/r/120188/#review122118 ::: browser/extensions/deployment-checker/bootstrap.js:234 (Diff revision 1) > console.log("deployment-checker payload:"); > console.log(payload); no need to keep the payload being logged either, if it's for QA folks.. The payload can be acessed in about:telemetry
Attachment #8847187 -
Flags: review?(felipc) → review+
![]() |
Reporter | |
Comment 7•4 years ago
|
||
The reason why is that this is using toolkit/modules/Console.jsm which has: log: createDumper("log"), where createDumper is defined at http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/toolkit/modules/Console.jsm#472 This calls sendConsoleAPIMessage which presumably sends stuff to an actual console, and also unconditionally calls dumpMessage which calls console.dump(). This is defined as: this.dump = aConsoleOptions.dump || dump; so everything gets dumped out to the terminal by default unless you passed in different console options. And in this case your consoleID is "addon/deployment-checker@mozilla.org" so presumably your console came from either addon-sdk/source/lib/toolkit/loader.js or addon-sdk/source/lib/sdk/console/plain-text.js. The former passes no "dump" option. The latter passes "dump: print", which will use whatever the "print" function was that was passed to the PlainTextConsole. If you came via addon-sdk/source/lib/sdk/content/sandbox.js then the "print" function is null... Anyway, the upshot is that if I'm reading this stuff correctly and this is the addon SDK console, we probably should never use its log() in opt builds. :(
![]() |
Reporter | |
Comment 8•4 years ago
|
||
Or I guess the Console.jsm console. I don't know why it feels the need to dump everything to stdout. :(
![]() |
Reporter | |
Comment 9•4 years ago
|
||
And to the Android log. And I would bet that this dump is unconditional even in opt builds, because we're a sandbox, so it ignores the window-specific dump.enabled pref.
![]() |
Assignee | |
Comment 10•4 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847187 [details] bug 1347217 - lessen debug spew in deployment-checker https://reviewboard.mozilla.org/r/120188/#review122118 Thanks! I also switched `console.log` to `Services.console.logStringMessage` which doesn't appear to output to stdout/stderr. > no need to keep the payload being logged either, if it's for QA folks.. The payload can be acessed in about:telemetry Good call.
Comment hidden (mozreview-request) |
Comment 12•4 years ago
|
||
Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b28b0312ed4e lessen debug spew in deployment-checker r=Felipe
![]() |
Reporter | |
Comment 13•4 years ago
|
||
Thank you!
![]() |
||
Comment 14•4 years ago
|
||
I have been seeing this output too. Thank you for the fix.
Comment 15•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b28b0312ed4e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
![]() |
Assignee | |
Comment 17•4 years ago
|
||
Comment on attachment 8847187 [details] bug 1347217 - lessen debug spew in deployment-checker Approval Request Comment [Feature/Bug causing the regression]: bug 1346017 [User impact if declined]: unnecessary debug spew in the console [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: this lessens some debug output and changes which "console" the logging uses [String changes made/needed]: none
Attachment #8847187 -
Flags: approval-mozilla-release?
Attachment #8847187 -
Flags: approval-mozilla-beta?
Attachment #8847187 -
Flags: approval-mozilla-aurora?
Comment 18•4 years ago
|
||
Comment on attachment 8847187 [details] bug 1347217 - lessen debug spew in deployment-checker Remove unnecessary debug logs in the console. Aurora54+ & Beta53+.
Attachment #8847187 -
Flags: approval-mozilla-beta?
Attachment #8847187 -
Flags: approval-mozilla-beta+
Attachment #8847187 -
Flags: approval-mozilla-aurora?
Attachment #8847187 -
Flags: approval-mozilla-aurora+
Updated•4 years ago
|
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Comment 19•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/daf87d4151aa
Comment 20•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/da4e2f5a75dd
Comment 21•4 years ago
|
||
Setting qe-verify- based on David's assessment on manual testing needs (see Comment 17).
Flags: qe-verify-
Comment 22•4 years ago
|
||
Comment on attachment 8847187 [details] bug 1347217 - lessen debug spew in deployment-checker release52+ Will this change also be made on the xpi we're going to ship through gofaster?
Attachment #8847187 -
Flags: approval-mozilla-release? → approval-mozilla-release+
![]() |
Assignee | |
Comment 23•4 years ago
|
||
Yes, the xpi already has this change.
Comment 24•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-release/rev/0f3b930b3ef7
status-firefox52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•