Bug 28387 - Regression test for RegExp bug in v8
Summary: Regression test for RegExp bug in v8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-17 04:14 PDT by Christian Plesner Hansen
Modified: 2009-08-21 15:58 PDT (History)
1 user (show)

See Also:


Attachments
Initial version (2.40 KB, patch)
2009-08-17 04:18 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff
Relative to WebKit (2.54 KB, patch)
2009-08-17 22:32 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff
Let's try that again (2.83 KB, patch)
2009-08-17 23:35 PDT, Christian Plesner Hansen
eric: review+
Details | Formatted Diff | Diff
Results ordering (2.39 KB, patch)
2009-08-17 23:56 PDT, Christian Plesner Hansen
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Added trailing newline (2.39 KB, patch)
2009-08-19 07:38 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff
Fixed trailing newline issue (2.42 KB, patch)
2009-08-20 23:32 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Plesner Hansen 2009-08-17 04:14:16 PDT
Regression test for bug fixed in http://codereview.chromium.org/171039.  Regexps created in one frame were not callable outside that frame.
Comment 1 Christian Plesner Hansen 2009-08-17 04:18:44 PDT
Created attachment 34966 [details]
Initial version
Comment 2 Eric Seidel (no email) 2009-08-17 16:24:45 PDT
Comment on attachment 34966 [details]
Initial version

AFAIK, CPH is not a committer, adding cq+
Comment 3 Eric Seidel (no email) 2009-08-17 17:16:50 PDT
Comment on attachment 34966 [details]
Initial version

Clearing flags on attachment: 34966

Committed r47399: <http://trac.webkit.org/changeset/47399>
Comment 4 Eric Seidel (no email) 2009-08-17 17:16:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Christian Plesner Hansen 2009-08-17 20:44:56 PDT
Sorry, I accidentally made this patch relative to LayoutTests rather than WebKit so it has landed one level too far out, creating trunk/fast/...  It will have to be rolled back.
Comment 6 Mark Rowe (bdash) 2009-08-17 22:13:25 PDT
Rolled out in <https://trac.webkit.org/changeset/47411>.
Comment 7 Christian Plesner Hansen 2009-08-17 22:32:53 PDT
Created attachment 35015 [details]
Relative to WebKit
Comment 8 Eric Seidel (no email) 2009-08-17 22:55:08 PDT
Comment on attachment 35015 [details]
Relative to WebKit

WebKit indent is 4 spaces.

If you want this reviewed, you'll need to mark it r=?

Also, official webkit style has { on a new line for function declarations.

Also, LayoutTests/fast/regex/regexp-cross-frame-callable.html should just be a standard JS test, which uses make-script-test-wrappers to generate the wrapper .html file from the TEMPLATE.html

You just write fast/regexp/resources/regexp-cross-frame-callable.js and let make-script-test-wrappers do its thing for you.

If this was marked r=? I woudl mark it r-.  Since it's not, I'm assuming this is just a work-in progress. :)
Comment 9 Christian Plesner Hansen 2009-08-17 23:02:18 PDT
> You just write fast/regexp/resources/regexp-cross-frame-callable.js and let
> make-script-test-wrappers do its thing for you.

I need at least two html pages to exercise the cross-frame aspect.  How would I do that?
Comment 10 Eric Seidel (no email) 2009-08-17 23:04:49 PDT
You can easily do:
var iframe = document.createElement('iframe');
iframe.src = "my_amazing_test_file_with_lazer_eyes.html";
document.body.appendChild(iframe);

Your current test uses body.onload, which I think won't even wait for the iframe load.  I'm not certain.

You mgiht have to use iframe.onload = doTest() anyway.
Comment 11 Christian Plesner Hansen 2009-08-17 23:35:25 PDT
Created attachment 35021 [details]
Let's try that again
Comment 12 Eric Seidel (no email) 2009-08-17 23:46:30 PDT
Comment on attachment 35021 [details]
Let's try that again

So this looks fine.

It could be one step better though.

Notice how the expected results are out of order:
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS re('a') is ['a']

This is because the script in the subframe is executing after the main frame finishes parsing.  Sadly, one ilmitation of the script-test testing framework is that we don't have a nice clean way to handle tests which need to delay completion.

I expect you could get around this pretty easily this way:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
iframe.document.body.innerHTML = "<script>top.doTest(/a/)</script>";

I would expect the script to execute synchronously from the innerHTML.

If not, certainly this should:
iframe.document.write("<script>top.doTest(/a/)</script>");

I'm OK committing this as-is, but I think we could improve it one step further if you're willing.  Either of the above should remove the need for a second file altogether.
Comment 13 Christian Plesner Hansen 2009-08-17 23:56:06 PDT
Created attachment 35023 [details]
Results ordering
Comment 14 Christian Plesner Hansen 2009-08-17 23:57:40 PDT
> I would expect the script to execute synchronously from the innerHTML.
> 
> If not, certainly this should:
> iframe.document.write("<script>top.doTest(/a/)</script>");

That's also a lot better since it removes the need for a separate file for the child frame.  I've fixed it.
Comment 15 Eric Seidel (no email) 2009-08-17 23:59:38 PDT
Comment on attachment 35023 [details]
Results ordering

Glorious!
Comment 16 Eric Seidel (no email) 2009-08-18 00:03:50 PDT
Comment on attachment 35023 [details]
Results ordering

Rejecting patch 35023 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 17 Eric Seidel (no email) 2009-08-18 00:20:48 PDT
Testing 11088 test cases.
fast/regex/cross-frame-callable.html -> failed

Exiting early after 1 failures.  7420 tests run.
131.57s total testing time

I did not look at the diffs.
Comment 18 Christian Plesner Hansen 2009-08-18 00:46:17 PDT
Where does this failure happen?  As far as I can tell it passes on safari and chrome with webkit latest and v8 latest.
Comment 19 Eric Seidel (no email) 2009-08-18 08:47:14 PDT
It happened when the commit-queue applied your patch, built (release) and ran the tests.  I didn't look at the diffs, but if you're not seeing this locally I can dig them up.
Comment 20 Christian Plesner Hansen 2009-08-18 10:52:38 PDT
(In reply to comment #19)
> It happened when the commit-queue applied your patch, built (release) and ran
> the tests.  I didn't look at the diffs, but if you're not seeing this locally I
> can dig them up.

Is there a way for me to run the tests in the same setup as the commit-queue?

I ran the test in safari on mac and there was no problem so if the diff is available somewhere that would be really helpful.
Comment 21 Eric Seidel (no email) 2009-08-18 14:24:55 PDT
The commit queue just runs "run-webkit-tests" which uses DumpRenderTree to run your test.
Comment 22 Christian Plesner Hansen 2009-08-19 07:38:04 PDT
Created attachment 35119 [details]
Added trailing newline

The problem is that there has to be an empty line at the end of the expectation file.  In my workspace the line is there but between creating and applying the patch it disappears.  This patch includes an extra newline but I'm not sure it works -- when I tried applying the patch the newline still disappeared.
Comment 23 Christian Plesner Hansen 2009-08-20 23:32:33 PDT
Created attachment 38363 [details]
Fixed trailing newline issue

Removing the iframe doesn't make any difference, there's still a trailing newline.  Only workaround I could come up with was to write DONE after the test was over, that seems to work.  If it turns out not to work for some reason I'm ready to just drop this CL altogether.
Comment 24 Eric Seidel (no email) 2009-08-21 14:59:21 PDT
Comment on attachment 38363 [details]
Fixed trailing newline issue

I'm not sure I know what's going on here, why "done" would be needed.  But I also think this is fine.
Comment 25 Eric Seidel (no email) 2009-08-21 15:57:58 PDT
Comment on attachment 38363 [details]
Fixed trailing newline issue

Clearing flags on attachment: 38363

Committed r47647: <http://trac.webkit.org/changeset/47647>
Comment 26 Eric Seidel (no email) 2009-08-21 15:58:01 PDT
All reviewed patches have been landed.  Closing bug.