Last Comment Bug 743615 - ImageData structured clone is broken (Peacekeeper)
: ImageData structured clone is broken (Peacekeeper)
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
http://peacekeeper.futuremark.com/
Depends on: 777628
Blocks: peacekeeper 550309
  Show dependency treegraph
 
Reported: 2012-04-08 23:57 PDT by Zlip792
Modified: 2012-09-04 12:18 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Part 1 - Add the JS_{Read,Write}StructuredClone api. v1 (4.12 KB, patch)
2012-04-20 09:49 PDT, Bobby Holley (PTO through June 13)
jorendorff: review+
Details | Diff | Review
Part 2 - Unify structured clone tag enum for main thread and workers. v1 (7.17 KB, patch)
2012-04-20 09:50 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review-
Details | Diff | Review
Part 3 - Handle ImageData in the main thread runtime callbacks. v1 (4.57 KB, patch)
2012-04-20 09:50 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
Part 4 - MainThreadWorkerStructuredCloneCallbacks should not call the WorkerStructuredCloneCallbacks variants. v1 (1.52 KB, patch)
2012-04-20 09:50 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review+
Details | Diff | Review
Part 5 - Introduce the ImageData object/constructor in workers. v1 (7.73 KB, patch)
2012-04-20 09:52 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
Part 6 - Hook up worker ImageData to the structured clone stream. v1 (2.71 KB, patch)
2012-04-20 09:52 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review+
Details | Diff | Review
Part 7 - Tests. v1 (5.77 KB, patch)
2012-04-20 09:53 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review+
Details | Diff | Review
Part 3 - Handle ImageData in the main thread runtime callbacks. v2 (4.49 KB, patch)
2012-04-22 06:01 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review+
Details | Diff | Review
Part 5 - Introduce the ImageData object/constructor in workers. v2 (8.28 KB, patch)
2012-04-22 06:03 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review+
Details | Diff | Review
rollup patch v1 (31.67 KB, patch)
2012-04-22 06:14 PDT, Bobby Holley (PTO through June 13)
blassey.bugs: approval‑mozilla‑central+
Details | Diff | Review
Part 2 - Base worker structured clone tags on the dom ones to ensure against collisions. v1 (1.13 KB, patch)
2012-04-23 15:34 PDT, Bobby Holley (PTO through June 13)
bent.mozilla: review+
Details | Diff | Review

Description Zlip792 2012-04-08 23:57:36 PDT
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.
Comment 1 Zlip792 2012-04-09 05:04:32 PDT
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.
Comment 2 Zlip792 2012-04-09 06:35:34 PDT
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
Comment 3 Alice0775 White 2012-04-09 06:45:23 PDT
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
Comment 4 Boris Zbarsky [:bz] 2012-04-09 06:52:13 PDT
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?
Comment 5 Boris Zbarsky [:bz] 2012-04-09 07:36:06 PDT
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.
Comment 7 Alex Keybl [:akeybl] 2012-04-16 16:34:59 PDT
(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?
Comment 8 Boris Zbarsky [:bz] 2012-04-16 19:32:30 PDT
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.
Comment 9 Bobby Holley (PTO through June 13) 2012-04-17 13:26:36 PDT
Taking.
Comment 10 Bobby Holley (PTO through June 13) 2012-04-20 09:49:30 PDT
Created attachment 617008 [details] [diff] [review]
Part 1 - Add the JS_{Read,Write}StructuredClone api. v1

Attaching the clone JSAPI changes. Flagging jorendorff.
Comment 11 Bobby Holley (PTO through June 13) 2012-04-20 09:50:00 PDT
Created attachment 617009 [details] [diff] [review]
Part 2 - Unify structured clone tag enum for main thread and workers. v1

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.
Comment 12 Bobby Holley (PTO through June 13) 2012-04-20 09:50:25 PDT
Created attachment 617010 [details] [diff] [review]
Part 3 - Handle ImageData in the main thread runtime callbacks. v1
Comment 13 Bobby Holley (PTO through June 13) 2012-04-20 09:50:42 PDT
Created attachment 617011 [details] [diff] [review]
Part 4 - MainThreadWorkerStructuredCloneCallbacks should not call the WorkerStructuredCloneCallbacks variants. v1

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.
Comment 14 Bobby Holley (PTO through June 13) 2012-04-20 09:52:35 PDT
Created attachment 617013 [details] [diff] [review]
Part 5 - Introduce the ImageData object/constructor in workers. v1

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 15 Bobby Holley (PTO through June 13) 2012-04-20 09:52:54 PDT
Created attachment 617014 [details] [diff] [review]
Part 6 - Hook up worker ImageData to the structured clone stream. v1
Comment 16 Bobby Holley (PTO through June 13) 2012-04-20 09:53:33 PDT
Created attachment 617015 [details] [diff] [review]
Part 7 - Tests. v1
Comment 17 Bobby Holley (PTO through June 13) 2012-04-20 09:54:09 PDT
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 18 :Ms2ger 2012-04-20 09:57:07 PDT
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
Comment 19 Boris Zbarsky [:bz] 2012-04-20 10:27:08 PDT
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.
Comment 20 Bobby Holley (PTO through June 13) 2012-04-22 05:59:54 PDT
(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.
Comment 21 Bobby Holley (PTO through June 13) 2012-04-22 06:01:39 PDT
Created attachment 617309 [details] [diff] [review]
Part 3 - Handle ImageData in the main thread runtime callbacks. v2

Updated part 3 with a null check removed (thanks Ms2ger).
Comment 22 Bobby Holley (PTO through June 13) 2012-04-22 06:03:08 PDT
Created attachment 617310 [details] [diff] [review]
Part 5 - Introduce the ImageData object/constructor in workers. v2

Updated part 5 with readonly setter stuff sorted out.
Comment 23 Bobby Holley (PTO through June 13) 2012-04-22 06:14:40 PDT
Created attachment 617312 [details] [diff] [review]
rollup patch v1

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.
Comment 24 Bobby Holley (PTO through June 13) 2012-04-22 06:43:43 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b0a9c466eb74
Comment 25 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-23 12:58:06 PDT
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 26 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-23 13:12:20 PDT
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 27 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-23 13:26:06 PDT
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 28 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-23 13:30:16 PDT
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 29 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-23 13:41:09 PDT
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 30 Jason Orendorff [:jorendorff] 2012-04-23 14:31:38 PDT
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.
Comment 31 Bobby Holley (PTO through June 13) 2012-04-23 14:50:44 PDT
(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).
Comment 32 Bobby Holley (PTO through June 13) 2012-04-23 15:24:50 PDT
(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.
Comment 33 Bobby Holley (PTO through June 13) 2012-04-23 15:34:45 PDT
Created attachment 617676 [details] [diff] [review]
Part 2 - Base worker structured clone tags on the dom ones to ensure against collisions. v1

Updated part 2 - flagging bent for review.
Comment 35 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-23 16:14:11 PDT
This broke all of the builds, and got backed out because of it: https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd9f2f5529f
Comment 36 Jason Orendorff [:jorendorff] 2012-04-23 18:28:35 PDT
(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.
Comment 38 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-30 16:06:19 PDT
untracking, since this made it into Aurora 14.
Comment 39 Steve Fink [:sfink] [:s:] 2012-09-04 12:18:55 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.