Bug 22464 – Add a test for a potential crash in same-origin checks

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Pam Greene (IRC:pamg)]

Test that same-origin checks don't try to initialize the context. Doing so could cause a crash.

[Pam Greene (IRC:pamg)]

Created attachment 25442 [details]
New test + result

In the interest of full disclosure, I know very little about the underlying problem here, but the test looks straightforward... and crashing is never success. :)

[Darin Adler]

Comment on attachment 25442 [details]
New test + result

The test would be better if it made some visible change to the page inside the check_blank function just before calling notifyDone. We normally write out "PASS" in code like that.

That avoids having the test seem to succeed if some bug prevents the code in the test from running at all.

I'm going to say review+, but the test would be better with that small refinement.

Also, I'm not sure where the 100ms timeout comes from. Would the test not work with a 0 timeout?

[Sam Weinig]

What is the 'context' that the test is referring to?  Is there a particular patch that this was associated with?

[Pam Greene (IRC:pamg)]

(In reply to comment #3)
> What is the 'context' that the test is referring to?  Is there a particular
> patch that this was associated with?

This was for a bug in V8, contributed here on the "more tests can't be bad" philosophy.  The crash was exposed when trying to upload to orkut.com from a debug build of Chrome.

My impression from the bug report and checkin comments is that when performing a same-origin check, they were trying to initialize the JS execution context of the target frame in order to get its security token, but allocation is disallowed in security checks, so the app aborted.

To be quite frank, I have only an approximate idea what the previous paragraph means. But if some other text description would be clearer in the test, I'm happy to change it.

[Sam Weinig]

Ok, than the crash seems very v8 specific, we don't even use the Security token concept.  Having more tests is good, but I think the test's text should reflect what it is testing.  In this case, the text and name should not indicate that it is testing some sort of initialization of contexts, as that is not what is happening.

[Pam Greene (IRC:pamg)]

Created attachment 25458 [details]
Addressing Darin's and Sam's comments

Added "PASS" text, cut timeout to 0, reworded description, and renamed test.

[Darin Adler]

Comment on attachment 25458 [details]
Addressing Darin's and Sam's comments

This looks great. My only concern is that changing the timers to 0-duration might have made this so it wouldn't crash any more back with the original bug. It might be more reliable to use an onload handler on the iframe instead.

r=me as is, but this may need to be tested at some point to see if it still exercises the bug with a 0 timeout.

[Pam Greene (IRC:pamg)]

I ran the test with 0-duration timeouts on a pre-bugfix build of Chromium and verified that it still crashes.

Landed as r38880.