Defer iframe JavaScript URL evaluation

Make StyleElement robust against tree mutation

It is possible that HTMLStyleElement::removedFrom() is called
before HTMLStyleElement::didNotifySubtreeInsertionsToDocument().

BUG=356653
TEST=append-child-style-crash.html,javascript-url-style-crash.html
R=esprehn@chromium.org, abath@chromium.org

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

[Hajime Morrita, 2014-04-02 00:44:47]

PTAL?

I know this isn't ideal, but I have no better idea.
This is severe :-(

[abarth-chromium, 2014-04-02 00:54:57]

https://codereview.chromium.org/221673003/diff/1/LayoutTests/fast/frames/adop...
File LayoutTests/fast/frames/adopt-from-created-document.html (right):

https://codereview.chromium.org/221673003/diff/1/LayoutTests/fast/frames/adop...
LayoutTests/fast/frames/adopt-from-created-document.html:17: </script>
I'm worried that we're going to break web sites with this change.  Previously
the JavaScript URL executed before appendChild returned.  It looks like you've
changed it to execute at the end of the script...

[esprehn, 2014-04-02 00:59:32]

This doesn't seem right, why is a javascript url any different than an inline
<script> ? That also runs inside didNotifySubtreeInsertonsToDocument(). There's
no racing down there, it's all in tree order.

[abarth-chromium, 2014-04-02 01:02:17]

https://codereview.chromium.org/221673003/diff/1/Source/core/html/HTMLFrameJa...
File Source/core/html/HTMLFrameJavaScriptURLOpener.cpp (right):

https://codereview.chromium.org/221673003/diff/1/Source/core/html/HTMLFrameJa...
Source/core/html/HTMLFrameJavaScriptURLOpener.cpp:55:
m_element->setNameAndOpenURL();
This function eventually calls isURLAllowed(), which does stack inspector to
determine whether the JavaScript URL is allowed.  You're passing the security
tests, which suggests that moving this call to this point in time doesn't cause
a security problem, but I'm unsure why that is.  I guess the microtask
checkpoint happens before we take the current script off the stack?  What if one
script calls another?  Is it possible to dispatch an event synchronously in
another frame that causes a microtask checkpoint to occur while some other
script is on the stack instead of the one that authored the JavaScript URL?

[esprehn, 2014-04-02 01:07:13]

not lgtm, anything you can do in appendChild(<iframe src=javascript:...>) you
can also do in appendChild(<script>...</script>) so defering this doesn't
actually fix your security bug.

[Hajime Morrita, 2014-04-02 01:31:24]

PTAL?

[abarth-chromium, 2014-04-02 05:46:05]

https://codereview.chromium.org/221673003/diff/10001/LayoutTests/fast/frames/...
File LayoutTests/fast/frames/javascript-url-style-crash-expected.txt (right):

https://codereview.chromium.org/221673003/diff/10001/LayoutTests/fast/frames/...
LayoutTests/fast/frames/javascript-url-style-crash-expected.txt:2: PASS unless
carsh
carsh -> crash

https://codereview.chromium.org/221673003/diff/10001/Source/core/html/HTMLSty...
File Source/core/html/HTMLStyleElement.cpp (right):

https://codereview.chromium.org/221673003/diff/10001/Source/core/html/HTMLSty...
Source/core/html/HTMLStyleElement.cpp:214: return;
Do other didNotifySubtreeInsertionsToDocument assume they're in the document? 
Maybe we shouldn't even call this function if the element isn't in the document
anymore.

[Hajime Morrita, 2014-04-02 17:01:37]

On 2014/04/02 05:46:05, abarth wrote:
>
https://codereview.chromium.org/221673003/diff/10001/Source/core/html/HTMLSty...
> Source/core/html/HTMLStyleElement.cpp:214: return;
> Do other didNotifySubtreeInsertionsToDocument assume they're in the document? 
> Maybe we shouldn't even call this function if the element isn't in the
document
> anymore.
This is good question. I filed a bug for tracking this. http://crbug.com/359148

[Hajime Morrita, 2014-04-02 17:19:32]

Fixed typo.

[esprehn, 2014-04-02 19:33:30]

https://codereview.chromium.org/221673003/diff/30001/Source/core/html/HTMLSty...
File Source/core/html/HTMLStyleElement.cpp (right):

https://codereview.chromium.org/221673003/diff/30001/Source/core/html/HTMLSty...
Source/core/html/HTMLStyleElement.cpp:214: return;
This is impossible, we only call this if you're in the document:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[eseidel, 2014-04-02 19:36:51]

I looked at async loads last week and convinced myself that
<iframe src="javascript..."> loaded async in FF.  I'd be surprised if that was a
compat concern.
https://codereview.chromium.org/210253003/

Looks like it never landed, sadly.

[eseidel, 2014-04-02 19:37:30]

javascript: urls are sync in Chrome, but async in FF:
https://codereview.chromium.org/210253003/patch/1/10003

[Hajime Morrita, 2014-04-02 19:41:36]

On 2014/04/02 19:36:51, eseidel wrote:
> I looked at async loads last week and convinced myself that
> <iframe src="javascript..."> loaded async in FF.  I'd be surprised if that was
a
> compat concern.

This is good news. We could kill this then.

[Hajime Morrita, 2014-04-02 19:42:30]

On 2014/04/02 19:33:30, esprehn wrote:
>
https://codereview.chromium.org/221673003/diff/30001/Source/core/html/HTMLSty...
> Source/core/html/HTMLStyleElement.cpp:214: return;
> This is impossible, we only call this if you're in the document:
Oh right. Removed. PTAL?

[esprehn, 2014-04-02 19:45:10]

lgtm, but I don't think you want setTimeout(0)

https://codereview.chromium.org/221673003/diff/50001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLScriptElement/append-child-style-crash.html
(right):

https://codereview.chromium.org/221673003/diff/50001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLScriptElement/append-child-style-crash.html:22: }, 0);
ditto

https://codereview.chromium.org/221673003/diff/50001/LayoutTests/fast/frames/...
File LayoutTests/fast/frames/javascript-url-style-crash.html (right):

https://codereview.chromium.org/221673003/diff/50001/LayoutTests/fast/frames/...
LayoutTests/fast/frames/javascript-url-style-crash.html:22: }, 0);
What are you trying to do with the setTimeout? Wait for a layout?

setTimeout is flaky, you can't guarantee anything will happen.

Do you want document.body.offsetTop to cause a layout?

[gmorrita, 2014-04-02 19:47:29]

https://codereview.chromium.org/221673003/diff/50001/LayoutTests/fast/frames/...
File LayoutTests/fast/frames/javascript-url-style-crash.html (right):

https://codereview.chromium.org/221673003/diff/50001/LayoutTests/fast/frames/...
LayoutTests/fast/frames/javascript-url-style-crash.html:22: }, 0);
On 2014/04/02 19:45:10, esprehn wrote:
> What are you trying to do with the setTimeout? Wait for a layout?
> 
> setTimeout is flaky, you can't guarantee anything will happen.
> 
> Do you want document.body.offsetTop to cause a layout?
Oops. This part just survived from my first change where I tried to turn
javascript: evaluation async.
Will remove setTimeout().

[Hajime Morrita, 2014-04-02 19:50:53]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-04-02 19:50:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/221673003/70001

[commit-bot: I haz the power, 2014-04-02 21:00:13]

Change committed as 170702