dump calls are not atomic

RESOLVED FIXED in mozilla13

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 599019 [details]
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.
Created attachment 599020 [details]
The worker used by the HTML - must be next to the HTML file
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...
Hardware: x86_64 → All
Created attachment 599377 [details] [diff] [review]
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.
Assignee: nobody → mhammond
Attachment #599377 - Flags: review?(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?
> Have you ran this through try etc?

Not yet - time to learn a new trick :)
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

7 years ago
Or maybe we should have both dump() and dumperr()?
Is there really a need for both?
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.
Created attachment 603110 [details] [diff] [review]
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.
Attachment #599377 - Attachment is obsolete: true
Attachment #603110 - Flags: review?
Attachment #599377 - Flags: review?(jst)
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!
Attachment #603110 - Flags: review? → review+
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.
Status: NEW → ASSIGNED
OS: Windows Vista → All
Target Milestone: --- → mozilla13
I fixed this for JS modules/components in bug 695685, FWIW. I totally missed those other two dump implementations. :-/
https://hg.mozilla.org/mozilla-central/rev/24a298cde1e5
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.