Open Bug 1543115 Opened 6 months ago Updated 10 days ago

Accomodate to Puppeteer grepping stderr to fetch WebSocket server URL

Categories

(Remote Protocol :: Agent, defect, P1)

defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: ochameau, Assigned: ato)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

Puppeteer is grepping the stderr of chromium in order to find the WebSocket server URL that is printed on stderr by chromium in the following form:

$ chromium-browser --remote-debugging-port=9000
DevTools listening on ws://127.0.0.1:9000/devtools/browser/17d40cfd-563c-48c1-aee7-3abbf6d530c1

Here is the code in Puppeteer which does the grepping:
https://github.com/GoogleChrome/puppeteer/blob/2a7c3fe3e68aac3dbca130065a67a8f0aa7db621/lib/Launcher.js#L361

In bug 1539221, I ensured that the WebSocket URL was printed, but there is a couple of things to be done still:

  • Print the message on stderr:
    For now, this is printed on stdout whereas chromium prints that on stderr. Unfortunately I don't know about any JS API to print to stderr.
  • Always print this message:
    For now, this isn't displayed unless you change remote.log.level preference.
  • Change the message to match chromium one:
    We should replace "Remote debugging agent" with "DevTools".

Obviously, we could contribute to Puppeteer to match Firefox behavior, but that wouldn't help supporting Puppeteer old versions.

In the Puppeteer/Juggler sync meeting yesterday, the Puppeteer
developers mentioned that they were changing Puppeteer from scanning
stderr to scanning both stdout + stderr. It doesn’t look like that
has landed yet.

That doesn’t give us compatibility with earlier Puppeteer versions,
but as I understand it this isn’t a concern?

Otherwise I would be happy to write a patch for a new edump()
statement. Alternatively we could use a Rust/C++ component to print
to stderr for us.

Priority: -- → P3

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #1)

In the Puppeteer/Juggler sync meeting yesterday, the Puppeteer
developers mentioned that they were changing Puppeteer from scanning
stderr to scanning both stdout + stderr. It doesn’t look like that
has landed yet.

It isn't clear when it is going to be changed.
Today's upstream is still using stderr:
https://github.com/GoogleChrome/puppeteer/blob/master/lib/Launcher.js#L327

That doesn’t give us compatibility with earlier Puppeteer versions,
but as I understand it this isn’t a concern?

In the long term, yes, we shouldn't try to be compatible against old Puppeteer versions.

But as we start looking into existing test suites, it is easier to run against
whatever Puppeteer version the test suite is using.
It allows to abstract away failures that may come when upgrading to a more recent version
of Puppeteer. (https://github.com/WordPress/gutenberg/pull/14986)

Also, more concretely, it would allow using an unmodified version of puppeteer to run
against Firefox. For now, I do have to manually patch Launch.js from within node_modules folder.
See bug 1539202 comment 0.

Otherwise I would be happy to write a patch for a new edump()
statement. Alternatively we could use a Rust/C++ component to print
to stderr for us.

If that's easy to implement, it would help testing/running puppeteer in the short term.
(And who knows, it may benefit to other usecases outside of the remote agent?!)

If this is required to run the Puppeteer test suite out of the box,
we need to fix this before we can actually run it in automation.

Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: P3 → P1

Mossop: This patch introduces a chrome-only edump() global for
writing from JS to stderr. Is something along these lines a thing
we might possibly consider accepting into Gecko?

Attachment #9060962 - Flags: feedback?(dtownsend)
Comment on attachment 9060962 [details] [diff] [review]
0001-edump-proposal.patch

I think this seems like a reasonable thing to add. However I suspect your current fix won't work from JSM files since there are at least 7 different implementations of dump in tree, or at least that is how many I've found so far, each applying to different scopes. This attachment only works from code running in a DOM window.

I'm going back and forwards on whether we should implement edump for all the places that has dump since as mentioned this might be more widely usable, but since I'd prefer we not be logging stuff to the console generally I think just the one change makes more sense.

I think the thing you actually want to alter is this: https://searchfox.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#142
Attachment #9060962 - Flags: feedback?(dtownsend) → feedback+

(Though for goodness sake if you felt like writing a patch to unify the multitude of dump implementations I'd be down with that)

An alternative I played around with yesterday, is bootstrapping the
RemoteAgent JS component using Rust. The idea is to implement
nsICommandLineHandler in Rust, because it supports writing to
stderr, and then to call nsIRemoteAgent implemented in JS from
there.

This has the benefit over the edump() patch that it’s work we
would have to do regardless when making the move from httpd.js to
hyper, so there’s potential here for killing two birds with one
stone.

Depends on: 1537768
Depends on: 1551188
Priority: P1 → P2

Marking as P1 so the bug appears in the project's Bugzilla queries.

Priority: P2 → P1

(In reply to Marco Mucci [:MarcoM] from comment #8)

Marking as P1 so the bug appears in the project's Bugzilla queries.

Normally I only mark the thing I’m actually working on as P1.

Thanks for clarifying that. I'll mark it as P2 and available for selection.

Assignee: ato → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
No longer blocks: 1540655

Another reason not mentioned previously in this bug, and which I
was just painfully reminded of trying to get bug 1540655 landed,
is that dump() only works in debug builds.

That means that for shippable builds, we must even set
browser.dom.window.dump.enabled for the workaround to work!

This is unaccaptable UX and what I would characterise as a show-stopper
for the late-2019 MVP, so I will make it a priority of mine to fix
this as soon as possible.

Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: P2 → P1
See Also: → 1580470
You need to log in before you can comment on or make changes to this bug.