Run this benchmark: http://peacekeeper.futuremark.com/ It fails on "WebWorker" test. It can't proceed from here on. I think regression window is require since when this test start failing. Actual Result: Fail to proceed and show this error in Error Console: DataCloneErro: The object could not be cloned. null Expected Result: It should pass this thread.

Some one else also mentioned this failure in another bug, here its link: https://bugzilla.mozilla.org/show_bug.cgi?id=723481#c1 Which indicates failure on Linux also.

Initial try of finding regression window from mozilla-inbound win32 tinder box builds. Regressed in this pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fcf369a4f12b&tochange=372f90787ec8

Error: DataCloneError: The object could not be cloned. Source file: null Regression windowWorks http://hg.mozilla.org/mozilla-central/rev/e5f6caa40409 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120316 Firefox/14.0a1 ID:20120316031151 Fails http://hg.mozilla.org/mozilla-central/rev/ee56787a20fb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120316 Firefox/14.0a1 ID:20120316025529 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e5f6caa40409&tochange=ee56787a20fb

Is this test passing an ImageData object to workers or something? It's possible that ImageData objects used to be clonable before bug 550309, but I bet they're not anymore. And per the spec at http://www.w3.org/TR/html5/common-dom-interfaces.html#safe-passing-of-structured-data they're not supposed to be clonable. Is that what's going on? Are they clonable in other UAs?

OK, I just checked and that spec, as well as the more current version at http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#safe-passing-of-structured-data seems to not allow cloning typed arrays. At least unless they're an "Object object" (something the spec does not define). I also checked that both we and WebKit allow cloning typed arrays. WebKit also allows cloning ImageData objects. So it seems entirely plausible that we used to allow cloning ImageData objects, since they were just a plain object with a typed array member. We probably need to fix structured cloning to allow that again, and fix the spec to allow cloning both ImageData and typed arrays. Where do structured clone algorithm bugs go? I'll file issues on the spec... In any case, this is not worker-specific, since just doing postMessage to a Window is enough to reproduce the behavior.

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=16667 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=16669

(In reply to Boris Zbarsky (:bz) from comment #6) > Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=16667 and > https://www.w3.org/Bugs/Public/show_bug.cgi?id=16669 Before tracking for FF14 I'd like to clarify - is there still further action to be taken here on our side prior to release?

Yes. We still need to make structured cloning of ImageData work. It used to, the spec says it should, and this test is relying on it working.

Taking.

Attaching the clone JSAPI changes. Flagging jorendorff.

Rght now the sets are disjoint, but soon they won't be, since we use the same serialization for ImageData in both cases. Flagging bent for review.

Doing so makes no sense. It seems like a no-op right now because we handle all the same cases first. But it's a hazard, and a problem for my upcoming patches.

The one issue remaining here is that we need to make this not throw when assigning to the properties (ie, ditch js_GetterOnlyPropertyStub) to fix peacekeeper. Jorendorff is going to suggest how to do that. In the mean time though (I have to head out), here's the patch for review.

Comment on attachment 617009 [details] [diff] [review] Part 2 - Unify structured clone tag enum for main thread and workers. v1 Ok, that should be all of them. bent, this needs to land over the weekend for FF14, so if you can review it today that would be great.

Comment on attachment 617010 [details] [diff] [review] Part 3 - Handle ImageData in the main thread runtime callbacks. v1 Review of attachment 617010 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +3629,5 @@ > + // Construct the ImageData. > + nsCOMPtr<nsIDOMImageData> imageData = new ImageData(width, height, > + dataArray.toObject()); > + if (!imageData) { > + JS_ReportOutOfMemory(cx); No need, |new| is infallible on this side @@ +3669,5 @@ > + // Prepare the ImageData internals. > + PRUint32 width, height; > + JS::Value dataArray; > + if (NS_FAILED(imageData->GetWidth(&width)) || > + NS_FAILED(imageData->GetHeight(&height)) || I'd static_cast to mozilla::dom::ImageData and use the nice infallible getters

Also, that code will break silently (modulo whatever tests fail) when we move ImageData to new bindings. As a followup, we should at least add a utility method for "get an nsISupports from this JS object" that handles both PRIVATE_IS_NSISUPPORTS and IS_DOMJSCLASS cases correctly. Then we should change this code to use that method; we'll need it while the ImageDate binding is prefable.

(In reply to Ms2ger from comment #18) > Comment on attachment 617010 [details] [diff] [review] > Part 3 - Handle ImageData in the main thread runtime callbacks. v1 > > Review of attachment 617010 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsJSEnvironment.cpp > @@ +3629,5 @@ > > + // Construct the ImageData. > > + nsCOMPtr<nsIDOMImageData> imageData = new ImageData(width, height, > > + dataArray.toObject()); > > + if (!imageData) { > > + JS_ReportOutOfMemory(cx); > > No need, |new| is infallible on this side Right. > I'd static_cast to mozilla::dom::ImageData and use the nice infallible > getters I don't really want to, since for the data case that would circumvent the Object->Value conversion, and the wrapping.

Updated part 3 with a null check removed (thanks Ms2ger).

Updated part 5 with readonly setter stuff sorted out.

Attaching a rollup patch for pre-emptive approval, since bent has been bogged with other stuff and might get to this on the late side. [Approval Request Comment] Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval. Possible risk to Fennec Native 14 (if any): Risk situation identical on all platforms. In general, not very risky. It's a fair amount of code, but it's all code on a path where we currently have no story and just throw (ImageData in structured clone and workers). This is a regression from 13, and the fact that we broke support for this means that we break a major benchmark (Peacekeeper), which is bad. I think we want this for 14.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b0a9c466eb74

Comment on attachment 617009 [details] [diff] [review] Part 2 - Unify structured clone tag enum for main thread and workers. v1 Review of attachment 617009 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, I'd prefer to not do this.

Comment on attachment 617309 [details] [diff] [review] Part 3 - Handle ImageData in the main thread runtime callbacks. v2 Review of attachment 617309 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +3621,5 @@ > + uint32_t width, height; > + JS::Value dataArray; > + if (!JS_ReadUint32Pair(reader, &width, &height)) > + return NULL; > + if (!JS_ReadTypedArray(reader, &dataArray)) Nit: Below you use the pattern I like, why not here? E.g.: if (!JS_ReadUint32Pair(...) || !JS_ReadTypedArray(...)) return nsnull; @@ +3622,5 @@ > + JS::Value dataArray; > + if (!JS_ReadUint32Pair(reader, &width, &height)) > + return NULL; > + if (!JS_ReadTypedArray(reader, &dataArray)) > + return NULL; Nit: nsnull. @@ +3629,5 @@ > + // Construct the ImageData. > + nsCOMPtr<nsIDOMImageData> imageData = new ImageData(width, height, > + dataArray.toObject()); > + // Wrap it in a jsval. > + JSObject *global = JS_GetGlobalForScopeChain(cx); Nit: The style in this file is all kinds of inconsistent, but I think you should probably put * on the left and add braces to your single statement ifs.

Comment on attachment 617310 [details] [diff] [review] Part 5 - Introduce the ImageData object/constructor in workers. v2 Review of attachment 617310 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ImageData.cpp @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "ImageData.h" > + > +#include "jsapi.h" Nit: Not needed. @@ +41,5 @@ > + static JSObject* > + Create(JSContext* aCx, uint32_t aWidth, uint32_t aHeight, JSObject *aData) > + { > + MOZ_ASSERT(aData && js_IsTypedArray(aData) && > + JS_GetTypedArrayType(aData) == js::TypedArray::TYPE_UINT8_CLAMPED); Nit: I generally don't like multi-condition assertions because when they fail you can't tell in the logs which condition is bad. Please split into 3 assertions. @@ +59,5 @@ > + > + return obj; > + } > + > + static bool IsInstance(JSObject* aObj) Nit: Please keep 'static bool' on its own line, like 'Create' above. And for the other methods below. @@ +66,5 @@ > + } > + > + static uint32_t GetWidth(JSObject* aObj) > + { > + return JS_DoubleToUint32(JS_GetReservedSlot(aObj, SLOT_width).toNumber()); Nit: Please assert IsInstance first. And in the other methods below. ::: dom/workers/ImageData.h @@ +6,5 @@ > +#define mozilla_dom_workers_imagedata_h__ > + > +#include "Workers.h" > + > +#include "jspubtd.h" Not: Not needed

Comment on attachment 617014 [details] [diff] [review] Part 6 - Hook up worker ImageData to the structured clone stream. v1 Review of attachment 617014 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +85,5 @@ > #include "ScriptLoader.h" > #include "Worker.h" > #include "WorkerFeature.h" > #include "WorkerScope.h" > +#include "ImageData.h" Nit: in order please. @@ +343,5 @@ > + // Read the information out of the stream. > + uint32_t width, height; > + jsval dataArray; > + if (!JS_ReadUint32Pair(aReader, &width, &height)) > + return NULL; Nit: Here definitely brace single statement ifs

Comment on attachment 617015 [details] [diff] [review] Part 7 - Tests. v1 Review of attachment 617015 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/bugs/test_bug743615.html @@ +18,5 @@ > +</div> > +<pre id="test"> > +<script type="application/javascript"> > + > +// XXX - Test .data equality! Eh? @@ +64,5 @@ > + pattern = makePattern(imageData.data.length, 4, 3); > + setPattern(imageData, pattern); > + var worker = new Worker('worker_bug743615.js'); > + worker.onmessage = workerMessage; > + gFromWorker = true; This looks unused? @@ +70,5 @@ > +} > + > +function workerMessage(evt) { > + // Relay the results of the worker-side tests. > + is(evt.data.statusMessage, 'PASS', evt.data.statusMessage); That last arg should be a status message ::: dom/tests/mochitest/bugs/worker_bug743615.js @@ +10,5 @@ > + // Check against the interface object. > + if (!(imageData instanceof ImageData)) > + statusMessage = statusMessage + ", Bad interface object in worker"; > + > + // Make sure that writing to .data is a no-op when not in strict mode. Do you also want to test that it throws in strict mode? Also, we should probably test the getters for these too, yes?

Comment on attachment 617008 [details] [diff] [review] Part 1 - Add the JS_{Read,Write}StructuredClone api. v1 We talked about this -- this is a bug against spec, right? Cloning [img, img.data] is supposed to produce an array [A, B] such that A.data === B, right? And this won't, right? Either way, it looks totally fine as a low-risk Peacekeeper fix.

(In reply to Jason Orendorff [:jorendorff] from comment #30) > We talked about this -- this is a bug against spec, right? Cloning [img, > img.data] is supposed to produce an array [A, B] such that A.data === B, > right? > And this won't, right? Right. But I decided to go this route because it happens regardless of what we do with imagedata. If I have typed array foo, and do bar = clone([foo, foo]), bar[0] !== bar[1] (because the backreference stuff only lives in startObject and the like).

(In reply to ben turner [:bent] from comment #29) > > +function workerMessage(evt) { > > + // Relay the results of the worker-side tests. > > + is(evt.data.statusMessage, 'PASS', evt.data.statusMessage); > > That last arg should be a status message It is. > > ::: dom/tests/mochitest/bugs/worker_bug743615.js > @@ +10,5 @@ > > + // Check against the interface object. > > + if (!(imageData instanceof ImageData)) > > + statusMessage = statusMessage + ", Bad interface object in worker"; > > + > > + // Make sure that writing to .data is a no-op when not in strict mode. > > Do you also want to test that it throws in strict mode? We don't (a known bug with JSStrictPropertyOp). That could be a todo, but my hacky mechanism of communicating from the worker to the main thread doesn't really have a good mechanism for todos.

Updated part 2 - flagging bent for review.

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/834a69fbceb9 http://hg.mozilla.org/integration/mozilla-inbound/rev/9a9d8dccae44 http://hg.mozilla.org/integration/mozilla-inbound/rev/4eba9d8bda93 http://hg.mozilla.org/integration/mozilla-inbound/rev/6a1b82c1dbde http://hg.mozilla.org/integration/mozilla-inbound/rev/19165ab7879e http://hg.mozilla.org/integration/mozilla-inbound/rev/508082793803 http://hg.mozilla.org/integration/mozilla-inbound/rev/8019cb482108

This broke all of the builds, and got backed out because of it: https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd9f2f5529f

(In reply to Bobby Holley (:bholley) from comment #31) > (In reply to Jason Orendorff [:jorendorff] from comment #30) > > > We talked about this -- this is a bug against spec, right? Cloning [img, > > img.data] is supposed to produce an array [A, B] such that A.data === B, > > right? > > And this won't, right? > > Right. But I decided to go this route because it happens regardless of what > we do with imagedata. If I have typed array foo, and do bar = clone([foo, > foo]), bar[0] !== bar[1] (because the backreference stuff only lives in > startObject and the like). OK. Please file a follow-up bug and Cc me.

Fixed the rebase bustage and pushed directly to m-c so that this makes the uplift: http://hg.mozilla.org/mozilla-central/rev/bf3d41520582 http://hg.mozilla.org/mozilla-central/rev/620dde7a187c http://hg.mozilla.org/mozilla-central/rev/da2791698bf8 http://hg.mozilla.org/mozilla-central/rev/ed667638551d http://hg.mozilla.org/mozilla-central/rev/db555180be49 http://hg.mozilla.org/mozilla-central/rev/ced1395a8ad0 http://hg.mozilla.org/mozilla-central/rev/270848da27e4

untracking, since this made it into Aurora 14.

(In reply to Jason Orendorff [:jorendorff] from comment #36) > (In reply to Bobby Holley (:bholley) from comment #31) > > Right. But I decided to go this route because it happens regardless of what > > we do with imagedata. If I have typed array foo, and do bar = clone([foo, > > foo]), bar[0] !== bar[1] (because the backreference stuff only lives in > > startObject and the like). > > OK. Please file a follow-up bug and Cc me. Said followup bug appears to be bug 748309.