Closed Bug 728986 Opened 11 years ago Closed 11 years ago

dump calls are not atomic


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

Not set





(Reporter: markh, Assigned: markh)



(3 files, 1 obsolete file)

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
worker number 5
worker number 6
worker number 7
worker number 8
worker number 16
contewnotr kneurm bneurm b3e
worker number 18
wcoornkteern tn unmubmebre r1 49

worker number 20
wocroknetre nntu mnbuemrb e2r1
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.
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
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.
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 ( and I'll update the patch here once it's done.
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 - 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.
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. :-/
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.