675715 - Structured Clone should preserve identity of RegExp objects

Status: RESOLVED DUPLICATE of bug 748309

(Core :: JavaScript Engine, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

It's kind of edge-casey, but it's not particularly difficult to implement.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 549892 [details] [diff] [review]
Patch

Review of attachment 549892 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Kyle.  No r+ yet, but it'll be straightforward enough. I just have a question (and a few nits to pick).

::: js/src/jsclone.cpp
@@ +410,5 @@
> +{
> +    /*
> +     * Centralize handling of the "memory" in the structured clone algorithm
> +     */
> +    JS_ASSERT(obj && wasHandled);

Micro-nit: one-line comments are formatted like
  /* Centralize handling of the "memory" in the structured clone algorithm. */
Full stop at the end (yeah, we're that picky in js/src...)

Another micro-nit: Remove the assertion. Both obj and wasHandled are going to be dereferenced right away; in such cases it's house style not to assert.

@@ +416,5 @@
> +    *wasHandled = false;
> +
> +    CloneMemory::AddPtr p = memory.lookupForAdd(obj);
> +    if (p) {
> +        *wasHandled = true;

Another micro-nit: put *wasHandled = false after this if-block.

@@ +566,2 @@
>          } else if (obj->isDate()) {
>              jsdouble d = js_DateGetMsecSinceEpoch(context(), obj);

This is my main question about this patch: Why solve this problem only for RegExps?

Shouldn't it be an invariant that a structured clone only makes one copy of each object in the input, whether it's an Object, a RegExp, a Date, a typed array, or something else?

::: js/src/tests/js1_8_5/extensions/clone-complex-object.js
@@ +35,5 @@
>                  return false;
>  
> +            // This function is only designed to check Objects and Arrays and RegExps.
> +            assertEq(xcls === "[object Object]" || xcls === "[object Array]" ||
> +                     xcls === "[object RegExp]", true);

Well, the function isn't actually designed to check RegExps. Add this assertion right after the definition of isClone:

  assertEq(isClone(/x/, /y/), false);

That should pass, of course. isClone doesn't check that the two RegExps have the same pattern, so I think it'll fail.

  if (xcls === "[object RegExp]") {
      if (x.source !== y.source || x.global !== y.global || x.ignoreCase ...)
          return false;
  }

The reason the existing code doesn't already cover that is that we use Object.keys(), which only lists enumerable properties (the ones seen by a for-in loop).

Actually, instead of checking all 6 properties, it's probably better to say:

  if (xcls === "[object RegExp]") {
      if (x.toSource() !== y.toSource())
          return false;
  }

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> @@ +566,2 @@
> >          } else if (obj->isDate()) {
> >              jsdouble d = js_DateGetMsecSinceEpoch(context(), obj);
> 
> This is my main question about this patch: Why solve this problem only for
> RegExps?
> 
> Shouldn't it be an invariant that a structured clone only makes one copy of
> each object in the input, whether it's an Object, a RegExp, a Date, a typed
> array, or something else?

Excellent question.  We should do this for the other types as well.  I filed this bug as a result of the private email correspondence we had with the Chrome folks who pointed out this bug in particular.

Comment 3

[Steve Fink [:sfink] [:s:]]

Said followup bug appears to be bug 748309.