728986 - dump calls are not atomic

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: HTML file to load in the browser

When dump calls are made from Workers, the output is not atomic.  Thus, the dump output becomes garbled.  I'm attaching a test case which consists of 2 files - dump_content.html creates a new worker, then repeatedly calls 'dump':

      dump("content number " + i++ + "\n");

(where 'i' is obviously incremented each time.)  The worker does basically the exact same thing:

  dump("worker number " + i++ + "\n");

On running this, many of the dump calls are correct, however it is common to see them garbled - eg:

content number 0
content number 1
hello from the worker
worker number 0
worker number 1
...
contweonrtk neurm bneurm b2e
 4
worker number 5
worker number 6
...
worker number 7
worker number 8
...
worker number 16
contewnotr kneurm bneurm b3e
 17
worker number 18
wcoornkteern tn unmubmebre r1 49

worker number 20
wocroknetre nntu mnbuemrb e2r1
5
worker number 22
worker number 23
wcoornkteern tn unmubmebre r2 46
"""

Note in some cases each character of the dump() in the HTML is interspersed with each character of the dump in the worker.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: The worker used by the HTML - must be next to the HTML file

Comment 2

[Mark Hammond [:markh] [:mhammond]]

FWIW, I can't repro this on Linux and guess it will also work fine on Mac.  I'm seeing if I can convince Windows to set stderr to unbuffered to see if the problem goes away...

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: change nsGlobalWindow's dump to default to stderr

The root of this problem is that nsGlobalWindow's dump implementation writes to stdout whereas the chrome and worker dump impls write to stderr.  I'm attaching a simple patch to nsGlobalWindow which makes the problem go away.  It also has the nice side-effect of fixing a glitch in Jetpack's output handling (which doesn't currently have a bug)

Asking jst for review as he reviewed bug 489938 which also touched this same code - my apologies if there is someone more suited to a review.

Comment 4

[Johnny Stenback (:jst)]

Hey Mark, I have no problems with this change in general, but I'm somewhat worried that some of our test harnesses might be dependent on dump() output ending up in stdout instead of stderr. Have you ran this through try etc?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

> Have you ran this through try etc?

Not yet - time to learn a new trick :)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I suspect that there are far fewer callers depending on the behavior of the WorkerScope.cpp/XPCComponents.cpp than there are depending on nsGlobalWindow's. Given that XPCShell and nsGlobalWindow both currently use stdout, I think it makes more sense to try and change WorkerScope/XPCComponents implementations to use stdout.

Comment 7

[Jesse Ruderman]

Or maybe we should have both dump() and dumperr()?

Comment 8

[Johnny Stenback (:jst)]

Is there really a need for both?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

I don't see a reason for both, especially given some test scenarios don't even capture stderr.

FWIW, I've got a new patch that changes the other dumps to use stdout instead of stderr.  I've pushed this through a "small" try run and it seems to work fine.  I've got a full run now running (https://tbpl.mozilla.org/?tree=Try&rev=e63bfb0c19ab) and I'll update the patch here once it's done.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: change worker and chrome dumps to use stdout

A patch that changes sandbox and dom-worker dump() to write to stdout - this seems to have zero impact on the build/test infrastructure.  A try run with this patch is at https://tbpl.mozilla.org/?tree=Try&rev=b2c9de08fe86 - there is some orange there but I'm confident they aren't caused by this change.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 603110 [details] [diff] [review]
change worker and chrome dumps to use stdout

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

Looks fine to me!

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Pushed to inbound as 88313:24a298cde1e5.  Adjusting platform to "all" as even though the observed bug was only seen on Windows, the fix is a change on all platforms.

Comment 13

[(not currently active) Ted Mielczarek]

I fixed this for JS modules/components in bug 695685, FWIW. I totally missed those other two dump implementations. :-/

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/24a298cde1e5