Don't allow scripts to run when a node is being adopted.

Don't allow scripts to run when a node is being adopted.

Running a script in the middle of rescoping nodes can break a lot of invariants and leave DOM in an inconsistent state.

This patch adds a ScriptForbiddenScope in TreeScope::adoptIfNeeded to ensure it never happens.

BUG=605766
Committed: https://crrev.com/146ff1bb11c88778d466bd7a49b544f2b5806ee0
Cr-Commit-Position: refs/heads/master@{#389425}

[dominicc (has gone to gerrit), 2016-04-25 06:35:15]

dominicc@chromium.org changed reviewers:
+ dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-04-25 06:35:15]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-04-25 06:35:15]

lgtm

[commit-bot: I haz the power, 2016-04-25 06:35:31]

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

[commit-bot: I haz the power, 2016-04-25 07:46:52]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-25 07:48:22]

Description was changed from

==========
Don't allow scripts to run when a node is being adopted.

Running a script in the middle of rescoping nodes can break a lot of invariants
and leave DOM in an inconsistent state.

This patch adds a ScriptForbiddenScope in TreeScope::adoptIfNeeded to ensure it
never happens.

BUG=605766
==========

to

==========
Don't allow scripts to run when a node is being adopted.

Running a script in the middle of rescoping nodes can break a lot of invariants
and leave DOM in an inconsistent state.

This patch adds a ScriptForbiddenScope in TreeScope::adoptIfNeeded to ensure it
never happens.

BUG=605766

Committed: https://crrev.com/146ff1bb11c88778d466bd7a49b544f2b5806ee0
Cr-Commit-Position: refs/heads/master@{#389425}
==========

[commit-bot: I haz the power, 2016-04-25 07:48:23]

Patchset 1 (id:??) landed as
https://crrev.com/146ff1bb11c88778d466bd7a49b544f2b5806ee0
Cr-Commit-Position: refs/heads/master@{#389425}

[esprehn, 2016-04-25 20:55:21]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-04-25 20:55:22]

https://codereview.chromium.org/1910403003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1910403003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TreeScope.cpp:377: ScriptForbiddenScope
forbidScript;
This should be at the top of the function. ScriptForbiddenScope is like an
assert, needsScopeChange() should be able to run script either.

[esprehn, 2016-04-25 20:55:40]

On 2016/04/25 at 20:55:22, esprehn wrote:
>
https://codereview.chromium.org/1910403003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):
> 
>
https://codereview.chromium.org/1910403003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/TreeScope.cpp:377: ScriptForbiddenScope
forbidScript;
> This should be at the top of the function. ScriptForbiddenScope is like an
assert, needsScopeChange() should be able to run script either.

*shouldn't*

[Mariusz Mlynski, 2016-04-26 04:22:01]

On 2016/04/25 20:55:40, esprehn wrote:
> On 2016/04/25 at 20:55:22, esprehn wrote:
> >
>
https://codereview.chromium.org/1910403003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1910403003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/dom/TreeScope.cpp:377: ScriptForbiddenScope
> forbidScript;
> > This should be at the top of the function. ScriptForbiddenScope is like an
> assert, needsScopeChange() should be able to run script either.
> 
> *shouldn't*

Thanks for the feedback. This is true, |needsScopeChange()| shouldn't be allowed
to run script, neither should the TreeScopeAdopter constructor. I decided to put
the scope within the if block because |adoptIfNeeded| is called unconditionally
during various DOM operations and I didn't want to add another operation in the
hot path unless absolutely needed. |needsScopeChange()| does a comparison
operation only, so it's verified to be safe, and I figured the chances of it
becoming more complicated in the future are very low, hence the little
optimization.