Fix assertion failure in ScriptRegexp::match

Fix assertion failure in ScriptRegexp::match

BUG=517980
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201350

[yurys, 2015-08-27 22:39:49]

yurys@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, pfeldman@chromium.org

[yurys, 2015-08-27 22:39:50]

@esprehn can you change AllowUserAgentScript to work on all threads rather than
only on the main one?

[yurys, 2015-08-27 22:39:57]

The CQ bit was checked by yurys@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 22:40:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315413005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315413005/1

[commit-bot: I haz the power, 2015-08-27 23:18:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 23:18:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2015-08-27 23:46:40]

https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptRegexp.cpp (right):

https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptRegexp.cpp:73: allowScript.emplace();

Would it make sense to move the Optional into AllowUserAgentScript and make this
the default behavior of AllowUserAgentScript?

[yurys, 2015-08-27 23:56:49]

https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptRegexp.cpp (right):

https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptRegexp.cpp:73: allowScript.emplace();
On 2015/08/27 23:46:40, haraken wrote:
> 
> Would it make sense to move the Optional into AllowUserAgentScript and make
this
> the default behavior of AllowUserAgentScript?

Done. I'm not sure what was the reason to have all this machinery only on the
main thread.

[yurys, 2015-08-27 23:57:18]

The CQ bit was checked by yurys@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 23:57:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315413005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315413005/20001

[haraken, 2015-08-28 00:03:54]

LGTM

On 2015/08/27 23:56:49, yurys wrote:
>
https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
> File Source/bindings/core/v8/ScriptRegexp.cpp (right):
> 
>
https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
> Source/bindings/core/v8/ScriptRegexp.cpp:73: allowScript.emplace();
> On 2015/08/27 23:46:40, haraken wrote:
> > 
> > Would it make sense to move the Optional into AllowUserAgentScript and make
> this
> > the default behavior of AllowUserAgentScript?
> 
> Done. I'm not sure what was the reason to have all this machinery only on the
> main thread.

The reason is that the ScriptForbiddenScope is using a global variable (i.e.,
s_scriptForbiddenCount). Once we switch it to a thread-local storage, we can
remove the restriction about the main thread (and I think we should).

[yurys, 2015-08-28 00:22:32]

On 2015/08/28 00:03:54, haraken wrote:
> LGTM
> 
> On 2015/08/27 23:56:49, yurys wrote:
> >
>
https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
> > File Source/bindings/core/v8/ScriptRegexp.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
> > Source/bindings/core/v8/ScriptRegexp.cpp:73: allowScript.emplace();
> > On 2015/08/27 23:46:40, haraken wrote:
> > > 
> > > Would it make sense to move the Optional into AllowUserAgentScript and
make
> > this
> > > the default behavior of AllowUserAgentScript?
> > 
> > Done. I'm not sure what was the reason to have all this machinery only on
the
> > main thread.
> 
> The reason is that the ScriptForbiddenScope is using a global variable (i.e.,
> s_scriptForbiddenCount). Once we switch it to a thread-local storage, we can
> remove the restriction about the main thread (and I think we should).

I understand that, the question is why it was not thread-local from the very
beginning.

[yurys, 2015-08-28 00:22:34]

The CQ bit was unchecked by yurys@chromium.org

[yurys, 2015-08-28 00:22:38]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2015-08-28 00:22:47]

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

[haraken, 2015-08-28 00:23:20]

On 2015/08/28 00:22:32, yurys wrote:
> On 2015/08/28 00:03:54, haraken wrote:
> > LGTM
> > 
> > On 2015/08/27 23:56:49, yurys wrote:
> > >
> >
>
https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
> > > File Source/bindings/core/v8/ScriptRegexp.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1315413005/diff/1/Source/bindings/core/v8/Scr...
> > > Source/bindings/core/v8/ScriptRegexp.cpp:73: allowScript.emplace();
> > > On 2015/08/27 23:46:40, haraken wrote:
> > > > 
> > > > Would it make sense to move the Optional into AllowUserAgentScript and
> make
> > > this
> > > > the default behavior of AllowUserAgentScript?
> > > 
> > > Done. I'm not sure what was the reason to have all this machinery only on
> the
> > > main thread.
> > 
> > The reason is that the ScriptForbiddenScope is using a global variable
(i.e.,
> > s_scriptForbiddenCount). Once we switch it to a thread-local storage, we can
> > remove the restriction about the main thread (and I think we should).
> 
> I understand that, the question is why it was not thread-local from the very
> beginning.

I don't see a reason. As far as I know, ScriptForbiddenScope is not used in
performance-sensitive places. I think we can just try to make it thread-local
storage.

[commit-bot: I haz the power, 2015-08-28 01:06:57]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201350