242 bytes, text/html
823 bytes, patch
|Details | Diff | Splinter Review|
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.
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.
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.
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. :-/
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.