Bug 28387 – Regression test for RegExp bug in v8

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Christian Plesner Hansen]

Regression test for bug fixed in http://codereview.chromium.org/171039.  Regexps created in one frame were not callable outside that frame.

[Christian Plesner Hansen]

Created attachment 34966 [details]
Initial version

[Eric Seidel (no email)]

Comment on attachment 34966 [details]
Initial version

AFAIK, CPH is not a committer, adding cq+

[Eric Seidel (no email)]

Comment on attachment 34966 [details]
Initial version

Clearing flags on attachment: 34966

Committed r47399: <http://trac.webkit.org/changeset/47399>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Christian Plesner Hansen]

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.

[Mark Rowe (bdash)]

Rolled out in <https://trac.webkit.org/changeset/47411>.

[Christian Plesner Hansen]

Created attachment 35015 [details]
Relative to WebKit

[Eric Seidel (no email)]

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. :)

[Christian Plesner Hansen]

> 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?

[Eric Seidel (no email)]

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.

[Christian Plesner Hansen]

Created attachment 35021 [details]
Let's try that again

[Eric Seidel (no email)]

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.

[Christian Plesner Hansen]

Created attachment 35023 [details]
Results ordering

[Christian Plesner Hansen]

> 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.

[Eric Seidel (no email)]

Comment on attachment 35023 [details]
Results ordering

Glorious!

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Christian Plesner Hansen]

Where does this failure happen?  As far as I can tell it passes on safari and chrome with webkit latest and v8 latest.

[Eric Seidel (no email)]

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.

[Christian Plesner Hansen]

(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.

[Eric Seidel (no email)]

The commit queue just runs "run-webkit-tests" which uses DumpRenderTree to run your test.

[Christian Plesner Hansen]

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.

[Christian Plesner Hansen]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 38363 [details]
Fixed trailing newline issue

Clearing flags on attachment: 38363

Committed r47647: <http://trac.webkit.org/changeset/47647>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.