Bug 44718 - pushState and replaceState do not clone RegExp objects correctly
Summary: pushState and replaceState do not clone RegExp objects correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-26 13:44 PDT by Mihai Parparita
Modified: 2010-09-08 15:12 PDT (History)
12 users (show)

See Also:


Attachments
Patch for JSC::RegExp (7.36 KB, patch)
2010-08-26 17:36 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (24.77 KB, patch)
2010-08-27 11:15 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (21.49 KB, patch)
2010-08-27 18:37 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (16.58 KB, patch)
2010-09-07 13:47 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (16.56 KB, patch)
2010-09-07 14:29 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (16.56 KB, patch)
2010-09-07 15:18 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-08-26 13:44:10 PDT
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.
Comment 1 Mihai Parparita 2010-08-26 15:49:29 PDT
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)?
Comment 2 Oliver Hunt 2010-08-26 15:54:47 PDT
(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.
Comment 3 Mihai Parparita 2010-08-26 16:02:18 PDT
(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.
Comment 4 Mihai Parparita 2010-08-26 17:36:54 PDT
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.
Comment 5 Mihai Parparita 2010-08-27 11:15:59 PDT
Created attachment 65738 [details]
Patch
Comment 6 WebKit Review Bot 2010-08-27 11:17:27 PDT
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.
Comment 7 Mihai Parparita 2010-08-27 11:20:11 PDT
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.
Comment 8 Oliver Hunt 2010-08-27 11:34:19 PDT
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

?
Comment 9 Early Warning System Bot 2010-08-27 11:35:20 PDT
Attachment 65738 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3856030
Comment 10 WebKit Review Bot 2010-08-27 13:34:31 PDT
Attachment 65738 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3833036
Comment 11 Vitaly Repeshko 2010-08-27 15:03:23 PDT
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.
Comment 12 Mihai Parparita 2010-08-27 18:37:26 PDT
Created attachment 65800 [details]
Patch
Comment 13 WebKit Review Bot 2010-08-27 18:42:57 PDT
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.
Comment 14 Mihai Parparita 2010-08-27 18:45:31 PDT
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).
Comment 15 WebKit Review Bot 2010-08-27 19:39:54 PDT
Attachment 65738 [details] did not build on win:
Build output: http://queues.webkit.org/results/3871036
Comment 16 WebKit Review Bot 2010-08-28 09:21:31 PDT
Attachment 65800 [details] did not build on win:
Build output: http://queues.webkit.org/results/3853055
Comment 17 Mihai Parparita 2010-08-30 17:28:11 PDT
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).
Comment 18 Mihai Parparita 2010-09-07 13:47:25 PDT
Created attachment 66760 [details]
Patch
Comment 19 Mihai Parparita 2010-09-07 13:49:13 PDT
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?
Comment 20 Darin Adler 2010-09-07 13:49:54 PDT
Comment on attachment 66760 [details]
Patch

> +        RegExpRepresentation* const m_representation;

Can we use OwnPtr for this?
Comment 21 Mihai Parparita 2010-09-07 14:29:41 PDT
Created attachment 66766 [details]
Patch
Comment 22 Mihai Parparita 2010-09-07 14:30:14 PDT
(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.
Comment 23 WebKit Review Bot 2010-09-07 14:31:59 PDT
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.
Comment 24 Oliver Hunt 2010-09-07 14:37:08 PDT
Comment on attachment 66766 [details]
Patch

r=me!

Sorry i forgot to mention the serialization change in this bug earlier :(
Comment 25 Darin Adler 2010-09-07 14:37:51 PDT
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?
Comment 26 Mihai Parparita 2010-09-07 14:50:02 PDT
Comment on attachment 66766 [details]
Patch

Removing cq bit while I look at SunSpider impact.
Comment 27 Mihai Parparita 2010-09-07 15:18:52 PDT
Created attachment 66773 [details]
Patch
Comment 28 Mihai Parparita 2010-09-07 15:21:08 PDT
(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
Comment 29 WebKit Review Bot 2010-09-07 15:24:30 PDT
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.
Comment 30 Oliver Hunt 2010-09-07 15:26:04 PDT
(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)
Comment 31 Mihai Parparita 2010-09-07 15:43:20 PDT
$ 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%
Comment 32 Oliver Hunt 2010-09-07 15:44:06 PDT
Comment on attachment 66773 [details]
Patch

r=me
Comment 33 WebKit Commit Bot 2010-09-07 17:34:16 PDT
Comment on attachment 66773 [details]
Patch

Clearing flags on attachment: 66773

Committed r66936: <http://trac.webkit.org/changeset/66936>
Comment 34 WebKit Commit Bot 2010-09-07 17:34:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 WebKit Review Bot 2010-09-08 03:25:59 PDT
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
Comment 36 Mihai Parparita 2010-09-08 09:48:38 PDT
(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.
Comment 37 Eric Seidel (no email) 2010-09-08 15:00:24 PDT
Yes, looks flaky.  We need a bug on it.
Comment 38 Mihai Parparita 2010-09-08 15:12:41 PDT
(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?