Bug 44718 – pushState and replaceState do not clone RegExp objects correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Mihai Parparita]

pushState and replaceState are supposed to create a structured clone of the state object, and per http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#structured-clone that includes special-casing RegExp objects. Right now neither the JSC nor the V8 implementation of SerializedScriptValue implement this. They handle treating of RegExp as Object differently, which is why fast/loader/stateobjects/pushstate-object-types.html fails on Chromium.

[Mihai Parparita]

This involves using RegExp.h and RegExpObject.h from JavaScriptCore in WebCore. In turn, those depend on headers from JavaScriptCore/yarr. Is it really the right thing to be exporting all those headers from JavaScriptCore (I'm not that familiar with the forwarding header setup)?

[Oliver Hunt]

(In reply to comment #1)
> This involves using RegExp.h and RegExpObject.h from JavaScriptCore in WebCore. In turn, those depend on headers from JavaScriptCore/yarr. Is it really the right thing to be exporting all those headers from JavaScriptCore (I'm not that familiar with the forwarding header setup)?

Serialization is intrinsically dependent on the JS engine, you can't really avoid knowing internal details.

[Mihai Parparita]

(In reply to comment #2)
> Serialization is intrinsically dependent on the JS engine, you can't really avoid knowing internal details.

But all I want from the RegExp are the pattern and the flags (and to be able to create new RegExpObject instances), which shouldn't depend on internals.

If RegExp.h didn't include YARR headers (e.g. if the m_regExpJITCOde/mRegExpBytecode/m_regExp members were moved to a pimpl class that was forward declared in the header) then it seems like I could use that header without bringing in a lot of other things. I'm new WebKit (and even newer to JSC/bindings), so I don't know if that's a common pattern though.

[Mihai Parparita]

Created attachment 65651 [details]
Patch for JSC::RegExp

(In reply to comment #3)
> If RegExp.h didn't include YARR headers (e.g. if the m_regExpJITCOde/mRegExpBytecode/m_regExp members were moved to a pimpl class that was forward declared in the header) then it seems like I could use that header without bringing in a lot of other things. I'm new WebKit (and even newer to JSC/bindings), so I don't know if that's a common pattern though.

The attached patch implements that change (moves the JIT/bytecode/regexp to a RegExpRepresentation struct). If that seems reasonable, I can clean it up and send it out for review separately, so that the patch for this bug doesn't get too big.

[Mihai Parparita]

Created attachment 65738 [details]
Patch

[WebKit Review Bot]

Attachment 65738 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/RegExp.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihai Parparita]

Oliver, do you have time to review this? Let me know if you'd like me to split this into three patches (JavaScriptCore changes, JSC bindings changes, V8 bindings changes), though it seemed straightforward enough in its current state.

The style warning is about indentation inside the JSC namespace in JavaScriptCore/runtime/RegExp.h. I was just following the indentation level of the file. Let me know if you'd like to re-indent the whole file.

[Oliver Hunt]

Comment on attachment 65738 [details]
Patch

I'm slightly worried about the v8 serialisation -- what happens if i do:

RegExp.prototype.toString = function() { return "foo"; }


pushState(/a/) // or whatever

?

[Early Warning System Bot]

Attachment 65738 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3856030

[WebKit Review Bot]

Attachment 65738 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3833036

[Vitaly Repeshko]

A few comments on the V8 part:

> +    void writeRegExp(v8::Handle<v8::Value> value)
> +    {
> +        // We store RegExp values using their toString() representation (which
> +        // is then eval-ed in readRegExp) since V8 does not let us get at RegExp
> +        // internals directly.
> +        // FIXME: Change once http://code.google.com/p/v8/issues/detail?id=852
> +        // is fixed.
> +        append(RegExpTag);
> +        v8::String::Utf8Value regexpAsString(value->ToString());
> +        doWriteString(*regexpAsString, regexpAsString.length());
> +    }

You could get RegExp source and flags using Object::Get() and property names "source", "global", etc. (See ECMA-262 15.10.7.) Users can't override these properties. Of course, this doesn't help without a way to reconstruct an object.

> +    else if (value->IsRegExp())
> +        m_writer.writeRegExp(value);

Should probably call the other writeRegExp.

> +    bool readRegExp(v8::Handle<v8::Value>* value)
> +    {
> +        v8::Handle<v8::Value> regExpAsString;
> +        if (!readString(&regExpAsString))
> +            return false;
> +        v8::Local<v8::Script> regExpAsScript = v8::Script::New(regExpAsString->ToString());
> +        *value = regExpAsScript->Run();
> +        return true;
> +    }

I'll expose RegExp::New to simplify this. Basically "new RegExp(source, flags)" should be eval-ed here, where "source" and "flags" can be built using the above property values. The problem is that RegExp can be changed by the user in the current context, so for now to get the original builtin constructor we should eval "new /a/.constructor(source, flags)". This is probably too much trouble without the API support.

[Mihai Parparita]

Created attachment 65800 [details]
Patch

[WebKit Review Bot]

Attachment 65800 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/RegExp.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihai Parparita]

New version of the patch drops the V8 changes (filed bug 44809 about that).

It also adds forwarding headers that I'm hoping will fix the Qt and GTK compiles (I'm not 100% clear on what the intended purpose of files under ForwardingHeaders is).

[WebKit Review Bot]

Attachment 65738 [details] did not build on win:
Build output: http://queues.webkit.org/results/3871036

[WebKit Review Bot]

Attachment 65800 [details] did not build on win:
Build output: http://queues.webkit.org/results/3853055

[Mihai Parparita]

Oliver, would you be able to review this, now that the V8 changes have been moved out?

(In reply to comment #16)
> Attachment 65800 [details] did not build on win:
> Build output: http://queues.webkit.org/results/3853055

Unfortunately this output is not very helpful. I'm guessing that the list of exported symbols needs to be updated on the Windows side too. Looking at http://trac.webkit.org/log/trunk/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def, that seems to be something that's done after the fact as a build fix (I don't have easy access to a Windows machine).

[Mihai Parparita]

Created attachment 66760 [details]
Patch

[Mihai Parparita]

Latest version of the patch should work with the re-implementation of SerializedScriptValue done by r66850 (thanks for doing that, the new version is much less class-happy).

Oliver or Sam, could one of you review this?

[Darin Adler]

Comment on attachment 66760 [details]
Patch

> +        RegExpRepresentation* const m_representation;

Can we use OwnPtr for this?

[Mihai Parparita]

Created attachment 66766 [details]
Patch

[Mihai Parparita]

(In reply to comment #20)
> (From update of attachment 66760 [details])
> > +        RegExpRepresentation* const m_representation;
> 
> Can we use OwnPtr for this?

Makes sense; latest version of the patch does that.

[WebKit Review Bot]

Attachment 66766 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/RegExp.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 66766 [details]
Patch

r=me!

Sorry i forgot to mention the serialization change in this bug earlier :(

[Darin Adler]

Comment on attachment 66766 [details]
Patch

> +    , m_representation(adoptPtr(new RegExpRepresentation()))

No need for the parentheses here.

This means that every RegExp is using a second memory block. I’d expect that to slow down regular expression tests, perhaps even including the regular expression part of SunSpider. This is marked review+, but how can it be until we know the SunSpider impact?

[Mihai Parparita]

Comment on attachment 66766 [details]
Patch

Removing cq bit while I look at SunSpider impact.

[Mihai Parparita]

Created attachment 66773 [details]
Patch

[Mihai Parparita]

(In reply to comment #25)
> (From update of attachment 66766 [details])
> > +    , m_representation(adoptPtr(new RegExpRepresentation()))
> 
> No need for the parentheses here.

Removed in latest version of the patch.
 
> This means that every RegExp is using a second memory block. I’d expect that to slow down regular expression tests, perhaps even including the regular expression part of SunSpider. This is marked review+, but how can it be until we know the SunSpider impact?

I ran SunSpider in a release build with this change and compared it with the latest nightly build (at r66819), here are the averages for 5 runs:
    
           Total SunSpider time    regexp-dna time
  r66819:  263.14                  13.84
w/ patch:  266.9                   13.9

[WebKit Review Bot]

Attachment 66773 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/RegExp.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

(In reply to comment #28)
> (In reply to comment #25)
> > (From update of attachment 66766 [details] [details])
> > > +    , m_representation(adoptPtr(new RegExpRepresentation()))
> > 
> > No need for the parentheses here.
> 
> Removed in latest version of the patch.
> 
> > This means that every RegExp is using a second memory block. I’d expect that to slow down regular expression tests, perhaps even including the regular expression part of SunSpider. This is marked review+, but how can it be until we know the SunSpider impact?
> 
> I ran SunSpider in a release build with this change and compared it with the latest nightly build (at r66819), here are the averages for 5 runs:
> 
>            Total SunSpider time    regexp-dna time
>   r66819:  263.14                  13.84
> w/ patch:  266.9                   13.9

can you do

run-sunspider --runs 30 > oldTimes
apply patch
run-sunspider --runs 30 > newTimes

and paste the result of

sunspider-compare-results oldTimes newTimes

into the bug that will be more helpful (your numbers above indicate a ~1% regression, so i'd rather have stable numbers)

[Mihai Parparita]

$ sunspider-compare-results ~/Desktop/sunspider.before ~/Desktop/sunspider.after

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           -                 260.9ms +/- 0.3%   260.3ms +/- 0.2% 

=============================================================================

  3d:                  -                  35.4ms +/- 1.4%    35.4ms +/- 0.5% 
    cube:              ??                 12.3ms +/- 2.0%    12.4ms +/- 1.5%     not conclusive: might be *1.003x as slow*
    morph:             -                  11.1ms +/- 3.3%    10.9ms +/- 1.0% 
    raytrace:          *1.011x as slow*   12.0ms +/- 0.6%    12.1ms +/- 0.9%     significant

  access:              ??                 32.7ms +/- 0.8%    32.9ms +/- 0.9%     not conclusive: might be *1.004x as slow*
    binary-trees:      -                   3.2ms +/- 4.8%     3.2ms +/- 4.8% 
    fannkuch:          ??                 15.5ms +/- 1.2%    15.6ms +/- 1.2%     not conclusive: might be *1.004x as slow*
    nbody:             ??                  8.9ms +/- 1.1%     9.0ms +/- 1.3%     not conclusive: might be *1.004x as slow*
    nsieve:            ??                  5.1ms +/- 2.2%     5.1ms +/- 2.5%     not conclusive: might be *1.007x as slow*

  bitops:              ??                 20.8ms +/- 1.3%    20.9ms +/- 1.0%     not conclusive: might be *1.003x as slow*
    3bit-bits-in-byte: ??                  2.7ms +/- 6.5%     2.8ms +/- 5.8%     not conclusive: might be *1.025x as slow*
    bits-in-byte:      -                   7.1ms +/- 1.6%     7.1ms +/- 1.3% 
    bitwise-and:       -                   4.2ms +/- 3.8%     4.2ms +/- 3.6% 
    nsieve-bits:       ??                  6.8ms +/- 2.2%     6.9ms +/- 1.9%     not conclusive: might be *1.010x as slow*

  controlflow:         ??                  2.5ms +/- 7.5%     2.6ms +/- 7.0%     not conclusive: might be *1.039x as slow*
    recursive:         ??                  2.5ms +/- 7.5%     2.6ms +/- 7.0%     not conclusive: might be *1.039x as slow*

  crypto:              ??                 16.0ms +/- 1.4%    16.2ms +/- 1.4%     not conclusive: might be *1.008x as slow*
    aes:               ??                  9.2ms +/- 1.7%     9.2ms +/- 1.7%     not conclusive: might be *1.004x as slow*
    md5:               -                   3.8ms +/- 3.7%     3.8ms +/- 4.0% 
    sha1:              *1.044x as slow*    3.0ms +/- 0.0%     3.1ms +/- 4.1%     significant

  date:                -                  31.8ms +/- 1.3%    31.5ms +/- 0.9% 
    format-tofte:      -                  18.8ms +/- 1.8%    18.5ms +/- 1.4% 
    format-xparb:      -                  13.0ms +/- 1.3%    13.0ms +/- 0.5% 

  math:                -                  26.4ms +/- 0.7%    26.3ms +/- 0.7% 
    cordic:            -                   8.1ms +/- 1.4%     8.0ms +/- 0.9% 
    partial-sums:      -                  13.0ms +/- 0.0%    13.0ms +/- 0.0% 
    spectral-norm:     -                   5.3ms +/- 3.2%     5.3ms +/- 3.2% 

  regexp:              *1.026x as slow*   15.7ms +/- 1.1%    16.1ms +/- 0.6%     significant
    dna:               *1.026x as slow*   15.7ms +/- 1.1%    16.1ms +/- 0.6%     significant

  string:              1.013x as fast     79.5ms +/- 0.5%    78.5ms +/- 0.5%     significant
    base64:            -                   8.6ms +/- 2.2%     8.5ms +/- 2.2% 
    fasta:             1.026x as fast     10.6ms +/- 1.8%    10.3ms +/- 1.7%     significant
    tagcloud:          -                  20.4ms +/- 0.9%    20.2ms +/- 0.8% 
    unpack-code:       1.016x as fast     30.1ms +/- 0.8%    29.6ms +/- 0.6%     significant
    validate-input:    -                   9.8ms +/- 1.8%     9.8ms +/- 1.4%

[Oliver Hunt]

Comment on attachment 66773 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 66773 [details]
Patch

Clearing flags on attachment: 66773

Committed r66936: <http://trac.webkit.org/changeset/66936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66936 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/66936
http://trac.webkit.org/changeset/66935

[Mihai Parparita]

(In reply to comment #35)
> http://trac.webkit.org/changeset/66936 might have broken Leopard Intel Debug (Tests)
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/66936
> http://trac.webkit.org/changeset/66935

fast/files/workers/worker-read-blob-sync.html failed, but it has failed before and after these changes too. It looks to be flaky (though only on Leopard, since I can't reproduce locally). I filed bug 45396 about it.

[Eric Seidel (no email)]

Yes, looks flaky.  We need a bug on it.

[Mihai Parparita]

(In reply to comment #37)
> Yes, looks flaky.  We need a bug on it.

Filed bug 45396 for it. I guess we can skip it for Leopard for now?