ImageData structured clone is broken (Peacekeeper)

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Zlip792, Assigned: bholley)

Tracking

(Blocks: 1 bug, {regression})

unspecified
mozilla14
x86_64
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14-)

Details

(URL)

Attachments

(8 attachments, 3 obsolete attachments)

4.12 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.52 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
2.71 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
5.77 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
4.49 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
8.28 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
31.67 KB, patch
Details | Diff | Splinter Review
1.13 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 499198
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Updated

5 years ago
OS: Windows 7 → All
(Reporter)

Updated

5 years ago
Whiteboard: regression-window
(Reporter)

Updated

5 years ago
Keywords: regression
Whiteboard: regression-window
Keywords: regressionwindow-wanted
Component: General → DOM: Workers
QA Contact: general → workers
(Reporter)

Comment 2

5 years ago
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

5 years ago
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?
Blocks: 550309
Status: UNCONFIRMED → NEW
tracking-firefox14: --- → ?
Ever confirmed: true
Keywords: regressionwindow-wanted
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.
Component: DOM: Workers → DOM
QA Contact: workers → general
Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=16667 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=16669

Comment 7

5 years ago
(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.
Assignee: nobody → bobbyholley+bmo
Summary: Peacekeeper v2 "WebWorker" test is failing → ImageData structured clone is broken (Peacekeeper)
Created attachment 617008 [details] [diff] [review]
Part 1 - Add the JS_{Read,Write}StructuredClone api. v1

Attaching the clone JSAPI changes. Flagging jorendorff.
Attachment #617008 - Flags: review?(jorendorff)
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.
Attachment #617009 - Flags: review?(bent.mozilla)
Created attachment 617010 [details] [diff] [review]
Part 3 - Handle ImageData in the main thread runtime callbacks. v1
Attachment #617010 - Flags: review?(bent.mozilla)
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.
Attachment #617011 - Flags: review?(bent.mozilla)
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.
Attachment #617013 - Flags: review?(bent.mozilla)
Created attachment 617014 [details] [diff] [review]
Part 6 - Hook up worker ImageData to the structured clone stream. v1
Attachment #617014 - Flags: review?(bent.mozilla)
Created attachment 617015 [details] [diff] [review]
Part 7 - Tests. v1
Attachment #617015 - Flags: review?(bent.mozilla)
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.
Attachment #617009 - Attachment description: {art 2 - Unify structured clone tag enum for main thread and workers. v1 → Part 2 - Unify structured clone tag enum for main thread and workers. v1
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.
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).
Attachment #617010 - Attachment is obsolete: true
Attachment #617010 - Flags: review?(bent.mozilla)
Attachment #617309 - Flags: review?(bent.mozilla)
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.
Attachment #617310 - Flags: review?(bent.mozilla)
Attachment #617013 - Attachment is obsolete: true
Attachment #617013 - Flags: review?(bent.mozilla)
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.
Attachment #617312 - Flags: approval-mozilla-central?
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b0a9c466eb74
Attachment #617312 - Flags: approval-mozilla-central? → approval-mozilla-central+
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.
Attachment #617009 - Flags: review?(bent.mozilla) → review-
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.
Attachment #617309 - Flags: review?(bent.mozilla) → review+
Attachment #617011 - Flags: review?(bent.mozilla) → review+
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
Attachment #617310 - Flags: review?(bent.mozilla) → review+
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
Attachment #617014 - Flags: review?(bent.mozilla) → review+
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?
Attachment #617015 - Flags: review?(bent.mozilla) → review+
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.
Attachment #617008 - Flags: review?(jorendorff) → review+
(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.
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.
Attachment #617009 - Attachment is obsolete: true
Attachment #617676 - Flags: review?(bent.mozilla)
Attachment #617676 - Flags: review?(bent.mozilla) → 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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
untracking, since this made it into Aurora 14.
tracking-firefox14: ? → -
Depends on: 777628
(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.
You need to log in before you can comment on or make changes to this bug.