Closed
Bug 1272522
Opened 9 years ago
Closed 8 years ago
Handle native app output on stderr
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aswan, Assigned: aswan)
References
Details
(Whiteboard: [native messaging] triaged)
Attachments
(1 file, 2 obsolete files)
The Chrome documentation for native messaging says:
> When the native messaging host [...] writes to stderr [...], output is written to the error log of Chrome.
To implement this, we'll need a change to the interface to read() / readString() on the InputPipe class in subprocess so that we can do a read of an unspecified size (which should give us a promise that resolves immediately whenever data is available with whatever data is available at that moment).
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → aswan
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59570/
Attachment #8763408 -
Flags: review?(kmaglione+bmo)
Comment 2•8 years ago
|
||
Comment on attachment 8763408 [details]
Bug 1272522 Handle stderr of native app
https://reviewboard.mozilla.org/r/59570/#review56852
::: toolkit/components/extensions/NativeMessaging.jsm:274
(Diff revision 1)
> + return proc.stderr.readString().then(data => {
> + if (data.length == 0) {
> + // We have hit EOF, just stop reading
> + return;
> + }
> + Services.console.logStringMessage(`stderr output from native app ${app}: ${data}`);
This may a bit of overkill, but I think we should only log chunks up to the last `\n` (and strip any trailing `\r?\n`), and then buffer the rest for the next chunk.
It may not be an issue in practice, but since stderr is generally unbuffered, we're not unlikely to wind up with several chunks that make up a single line. I wouldn't be entirely surprised if we wound up getting stack traces that are bigger than the buffer size either, for that matter.
Also, I think this would be a bit easier to follow as an async function, as in [1], rather than as recursive promise handlers.
[1]: http://gecko.readthedocs.io/en/latest/toolkit/modules/subprocess/toolkit_modules/subprocess/index.html#process-and-pipe-lifecycles
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:580
(Diff revision 1)
> +
> + return {messages, result};
> + } finally {
> + Services.console.unregisterListener(listener);
> + }
> +});
Can you move this to `head.js`?
Attachment #8763408 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8763408 [details]
Bug 1272522 Handle stderr of native app
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59570/diff/1-2/
Attachment #8763408 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/59570/#review56852
> This may a bit of overkill, but I think we should only log chunks up to the last `\n` (and strip any trailing `\r?\n`), and then buffer the rest for the next chunk.
>
> It may not be an issue in practice, but since stderr is generally unbuffered, we're not unlikely to wind up with several chunks that make up a single line. I wouldn't be entirely surprised if we wound up getting stack traces that are bigger than the buffer size either, for that matter.
>
> Also, I think this would be a bit easier to follow as an async function, as in [1], rather than as recursive promise handlers.
>
> [1]: http://gecko.readthedocs.io/en/latest/toolkit/modules/subprocess/toolkit_modules/subprocess/index.html#process-and-pipe-lifecycles
Did you mean an actual js async function? My understanding is we don't support those yet, the bug to support them has not yet been resolved.
I rewrote it with Task.spawn() which does seem cleaner, apologies if I'm misunderstanding your comment...
Comment 5•8 years ago
|
||
Comment on attachment 8763408 [details]
Bug 1272522 Handle stderr of native app
https://reviewboard.mozilla.org/r/59570/#review58448
::: toolkit/components/extensions/NativeMessaging.jsm:274
(Diff revision 2)
> + let partial = "";
> + while (true) {
> + let data = yield proc.stderr.readString();
> + if (data.length == 0) {
> + // We have hit EOF, just stop reading
> + if (partial.length > 0) {
`if (partial) ...`
::: toolkit/components/extensions/NativeMessaging.jsm:281
(Diff revision 2)
> + }
> + break;
> + }
> +
> + let lines = data.split(/\r?\n/);
> + if (partial.length > 0) {
This check is unnecessary. If `partial` is an empty string, prepending it has no effect.
::: toolkit/components/extensions/test/mochitest/head.js:19
(Diff revision 2)
> }
> +
> +var promiseConsoleOutput = Task.async(function* (task) {
> + const DONE = "=== extension test console listener done ===";
> +
> + let listener, messages = [];
One declaration per line, please.
::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:575
(Diff revision 2)
> + yield extension.awaitMessage("finished");
> + yield extension.unload();
> + });
> +
> + let found = messages.filter(msg => msg.includes(STDERR_MSG));
> + is(found.length, 1, "Saw stderr message on the console");
Should write a multi-line message and make sure we get multiple messages.
Attachment #8763408 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8763408 [details]
Bug 1272522 Handle stderr of native app
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59570/diff/2-3/
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4379d1513fd
Handle stderr of native app r=kmag
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 8•8 years ago
|
||
Kris, I'm going to be offline most of the day, can you land this
if it looks okay? The first version of this broke since there was
a reference to Components.utils in head.js which is shared between
regular and chrome mochitests. There's probably a more elegant
way to handle this but this is just to fix the breakage in the
short term. I also need to look at the Windows failures, again
this is just to fix the breakage until then.
MozReview-Commit-ID: D5on2N7iMz7
Attachment #8767497 -
Flags: review?(kmaglione+bmo)
Comment 9•8 years ago
|
||
backed out for breaking tests like https://treeherder.mozilla.org/logviewer.html#?job_id=197468&repo=autoland
Flags: needinfo?(aswan)
Comment 10•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f11c5f94798a
Backed out changeset d4379d1513fd for failures in test_ext_alarms.html
Assignee | ||
Updated•8 years ago
|
Attachment #8767497 -
Attachment is obsolete: true
Attachment #8767497 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8763408 [details]
Bug 1272522 Handle stderr of native app
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59570/diff/3-4/
Comment 12•8 years ago
|
||
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1aa56f978ec8
Handle stderr of native app r=kmag
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(aswan)
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63004/
Attachment #8769031 -
Flags: review?(kmaglione+bmo)
Comment 14•8 years ago
|
||
bugherder |
Assignee | ||
Updated•8 years ago
|
Attachment #8769031 -
Attachment is obsolete: true
Attachment #8769031 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Updated•8 years ago
|
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•