Fix PPB_Flash_MessageLoop.

Fix PPB_Flash_MessageLoop.

This CL suspends script callbacks and resource loads while running nested message loop using PPB_Flash_MessageLoop.

BUG=569496
Committed: https://crrev.com/dd77c2a41c72589d929db0592565125ca629fb2c
Cr-Commit-Position: refs/heads/master@{#374529}

[yzshen1, 2016-01-04 23:52:37]

Description was changed from

==========
Suspend script callbacks and resource loads while running nested message loop
using PPB_Flash_MessageLoop.

BUG=569496
==========

to

==========
Suspend script callbacks and resource loads while running nested message loop
using PPB_Flash_MessageLoop.

BUG=569496
==========

[yzshen1, 2016-01-04 23:52:37]

yzshen@chromium.org changed reviewers:
+ jochen@chromium.org, natashenka@google.com, raymes@chromium.org

[yzshen1, 2016-01-04 23:56:01]

Hi, reviewers.

Please take a look. I verified that this CL does prevent the test case (included
in bug 565023) from showing a dialog with a different origin in it.

However, I haven't touched this part of code for a very long time, and I am not
experienced with blink/security. So, please review this CL very carefully. Thank
you!

[raymes, 2016-01-05 00:12:16]

lgtm for pepper but like you I know nothing of the underlying issue/fix so
please wait for someone more familiar with scripting/blink too :)

[jochen (gone - plz use gerrit), 2016-01-08 14:42:16]

Is it possible to add a test for this?

could you please also reformat the commit message to <80c and have a separate
summary in the first line?

[yzshen1, 2016-02-05 00:39:18]

Description was changed from

==========
Suspend script callbacks and resource loads while running nested message loop
using PPB_Flash_MessageLoop.

BUG=569496
==========

to

==========
Fix PPB_Flash_MessageLoop.

This CL suspends script callbacks and resource loads while running nested
message loop using PPB_Flash_MessageLoop.

BUG=569496
==========

[yzshen1, 2016-02-05 00:39:51]

On 2016/01/08 14:42:16, jochen wrote:
> Is it possible to add a test for this?
> 
> could you please also reformat the commit message to <80c and have a separate
> summary in the first line?

Sorry for the delay!

I finally figured out how to add a test for it. Please take another look,
reviewers. Thanks!

[jochen (gone - plz use gerrit), 2016-02-05 14:55:21]

lgtm

[raymes, 2016-02-08 00:46:26]

lgtm

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
File ppapi/tests/test_flash_message_loop.cc (right):

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
ppapi/tests/test_flash_message_loop.cc:148: ASSERT_TRUE(result_);
Will the script eventually be run and set the result_ to false? Is that hard to
test?

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
File ppapi/tests/test_flash_message_loop.h (right):

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
ppapi/tests/test_flash_message_loop.h:52: bool result_;
nit: maybe be a bit more specific and name this:
script_callback_has_run_? (you would need to invert the true/false in that case)

[raymes, 2016-02-08 00:46:43]

(also, sorry for the delay :)

[yzshen1, 2016-02-08 16:14:10]

Thanks Raymes!

natashenka: friendly ping.

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
File ppapi/tests/test_flash_message_loop.cc (right):

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
ppapi/tests/test_flash_message_loop.cc:148: ASSERT_TRUE(result_);
On 2016/02/08 00:46:26, raymes wrote:
> Will the script eventually be run and set the result_ to false? Is that hard
to
> test?

I did try to run the test without the fix, and the test failed.

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
File ppapi/tests/test_flash_message_loop.h (right):

https://codereview.chromium.org/1559113002/diff/20001/ppapi/tests/test_flash_...
ppapi/tests/test_flash_message_loop.h:52: bool result_;
On 2016/02/08 00:46:26, raymes wrote:
> nit: maybe be a bit more specific and name this:
> script_callback_has_run_? (you would need to invert the true/false in that
case)

I used it to report error for other failures such as exception in JS code. So I
changed it to "suspend_script_callback_result_".

[yzshen1, 2016-02-09 18:25:21]

yzshen@chromium.org changed reviewers:
+ jschuh@chromium.org

[yzshen1, 2016-02-09 18:26:26]

+Justin for security review since Natalie is on a trip

Please take a look, Justin. Thanks!

[jschuh, 2016-02-09 21:52:17]

Seems like the worst case is deadlock, so lgtm for security.

[yzshen1, 2016-02-09 21:53:59]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-02-09 21:53:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1559113002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-02-09 21:56:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559113002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559113002/40001

[commit-bot: I haz the power, 2016-02-09 23:37:23]

Description was changed from

==========
Fix PPB_Flash_MessageLoop.

This CL suspends script callbacks and resource loads while running nested
message loop using PPB_Flash_MessageLoop.

BUG=569496
==========

to

==========
Fix PPB_Flash_MessageLoop.

This CL suspends script callbacks and resource loads while running nested
message loop using PPB_Flash_MessageLoop.

BUG=569496
==========

[commit-bot: I haz the power, 2016-02-09 23:37:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-09 23:38:35]

Description was changed from

==========
Fix PPB_Flash_MessageLoop.

This CL suspends script callbacks and resource loads while running nested
message loop using PPB_Flash_MessageLoop.

BUG=569496
==========

to

==========
Fix PPB_Flash_MessageLoop.

This CL suspends script callbacks and resource loads while running nested
message loop using PPB_Flash_MessageLoop.

BUG=569496

Committed: https://crrev.com/dd77c2a41c72589d929db0592565125ca629fb2c
Cr-Commit-Position: refs/heads/master@{#374529}
==========

[commit-bot: I haz the power, 2016-02-09 23:38:36]

Patchset 3 (id:??) landed as
https://crrev.com/dd77c2a41c72589d929db0592565125ca629fb2c
Cr-Commit-Position: refs/heads/master@{#374529}