SVGTests should lazyAttach

SVGTests should lazyAttach

SVGTests::handleAttributeChange should use lazyAttach instead of calling
attach manually.

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

R=pdr@chromium.org

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

[pdr., 2013-05-18 03:37:59]

https://codereview.chromium.org/15398006/diff/1/Source/core/svg/SVGTests.cpp
File Source/core/svg/SVGTests.cpp (right):

https://codereview.chromium.org/15398006/diff/1/Source/core/svg/SVGTests.cpp#...
Source/core/svg/SVGTests.cpp:160: if (valid && !targetElement->attached())
You removed the parentNode check here which was added in
http://trac.webkit.org/changeset/126657 to fix a security bug. Can you help me
understand why that's safe?

[esprehn, 2013-05-18 03:45:59]

On 2013/05/18 03:37:59, pdr wrote:
> https://codereview.chromium.org/15398006/diff/1/Source/core/svg/SVGTests.cpp
> File Source/core/svg/SVGTests.cpp (right):
> 
>
https://codereview.chromium.org/15398006/diff/1/Source/core/svg/SVGTests.cpp#...
> Source/core/svg/SVGTests.cpp:160: if (valid && !targetElement->attached())
> You removed the parentNode check here which was added in
> http://trac.webkit.org/changeset/126657 to fix a security bug. Can you help me
> understand why that's safe?

lazyAttach just marks yourself as needing a style recalc, we don't go through
any of the attach logic, so it's impossible to end up with a detached parent
since you'll only ever go through attach() during the next style recalc (which
would have attached your parent before it even got to you).

[pdr., 2013-05-18 03:50:14]

On 2013/05/18 03:45:59, esprehn wrote:
> On 2013/05/18 03:37:59, pdr wrote:
> > https://codereview.chromium.org/15398006/diff/1/Source/core/svg/SVGTests.cpp
> > File Source/core/svg/SVGTests.cpp (right):
> > 
> >
>
https://codereview.chromium.org/15398006/diff/1/Source/core/svg/SVGTests.cpp#...
> > Source/core/svg/SVGTests.cpp:160: if (valid && !targetElement->attached())
> > You removed the parentNode check here which was added in
> > http://trac.webkit.org/changeset/126657 to fix a security bug. Can you help
me
> > understand why that's safe?
> 
> lazyAttach just marks yourself as needing a style recalc, we don't go through
> any of the attach logic, so it's impossible to end up with a detached parent
> since you'll only ever go through attach() during the next style recalc (which
> would have attached your parent before it even got to you).

Thanks for the explanation. This patch looks reasonable to me. LGTM.

[eseidel, 2013-05-18 05:50:12]

I think we should talk through the high-level arch that you're trying to
achieve.  Or we shoul djust land a zillion patches like this on a branch and
evaluate the whole picture?

[esprehn, 2013-05-18 06:26:11]

On 2013/05/18 05:50:12, eseidel wrote:
> I think we should talk through the high-level arch that you're trying to
> achieve.  Or we shoul djust land a zillion patches like this on a branch and
> evaluate the whole picture?

Landing on a branch just makes this super hard because I won't have bot coverage
or the commit queue, unless you just want to see the code?

The end result of this is that all ContainerNode methods that add nodes (ex.
appendChild or parserAppendChild) do the equivalent of lazyAttach for you if the
node is inDocument(). HTMLConstructionSite stops trying to make decisions about
attaching, and no one should call lazyAttach outside ContainerNode's
append/insert methods.

We can also then ASSERT(isDocumentNode() || document()->inStyleRecalc()) inside
Node::attach and everyone would be forbidden from manually trying to attach
(Document's are special).

This also reduces the "public API" of Node for attachment down to just
lazyReattach() and detach(). Everything else is handled for you by the
recalcStyle process.

Now that I finally understand everything that's going on here I can explain this
on a whiteboard or something too if you want, it all suddenly made sense to me.
:)

[commit-bot: I haz the power, 2013-05-21 19:38:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/15398006/1

[commit-bot: I haz the power, 2013-05-21 20:20:54]

Change committed as 150822

[eseidel, 2013-05-21 22:44:40]

This appears to be causing svg/custom/tearoffs-with-tearoffs.html to ASSERT.

[esprehn, 2013-05-21 22:46:21]

On 2013/05/21 22:44:40, eseidel wrote:
> This appears to be causing svg/custom/tearoffs-with-tearoffs.html to ASSERT.

Yeah, already rolled it out.

[esprehn, 2013-05-22 00:18:22]

On 2013/05/21 22:46:21, esprehn wrote:
> On 2013/05/21 22:44:40, eseidel wrote:
> > This appears to be causing svg/custom/tearoffs-with-tearoffs.html to ASSERT.
> 

This is because I jumped the gun on removing the parent()->attached() check.
https://codereview.chromium.org/15159008/ fixes lazyAttach to not lie about
being attached anymore, but until that lands we can't call lazyAttach() if your
parent is not attached() otherwise you'll hit this assert. I'll add it back and
land this again.

[pdr, 2013-05-22 00:21:48]

On 2013/05/22 00:18:22, esprehn wrote:
> On 2013/05/21 22:46:21, esprehn wrote:
> > On 2013/05/21 22:44:40, eseidel wrote:
> > > This appears to be causing svg/custom/tearoffs-with-tearoffs.html to
ASSERT.
> > 
> 
> This is because I jumped the gun on removing the parent()->attached() check.
> https://codereview.chromium.org/15159008/ fixes lazyAttach to not lie about
> being attached anymore, but until that lands we can't call lazyAttach() if
your
> parent is not attached() otherwise you'll hit this assert. I'll add it back
and
> land this again.

Can we wait for https://codereview.chromium.org/15159008/ to land and then land
this as-is?

[esprehn, 2013-05-22 00:28:13]

On 2013/05/22 00:21:48, pdr1 wrote:
> On 2013/05/22 00:18:22, esprehn wrote:
> > On 2013/05/21 22:46:21, esprehn wrote:
> > > On 2013/05/21 22:44:40, eseidel wrote:
> > > > This appears to be causing svg/custom/tearoffs-with-tearoffs.html to
> ASSERT.
> > > 
> > 
> > This is because I jumped the gun on removing the parent()->attached() check.
> > https://codereview.chromium.org/15159008/ fixes lazyAttach to not lie about
> > being attached anymore, but until that lands we can't call lazyAttach() if
> your
> > parent is not attached() otherwise you'll hit this assert. I'll add it back
> and
> > land this again.
> 
> Can we wait for https://codereview.chromium.org/15159008/ to land and then
land
> this as-is?

We could, but Eric has had me blocked on that patch for days. I'm going to come
back and fix up all the attached() checks anyway so this will get fixed in a
future patch.

[esprehn, 2013-05-22 00:31:29]

On 2013/05/22 00:28:13, esprehn wrote:
> ...
> > 
> > Can we wait for https://codereview.chromium.org/15159008/ to land and then
> land
> > this as-is?
> 
> We could, but Eric has had me blocked on that patch for days. I'm going to
come
> back and fix up all the attached() checks anyway so this will get fixed in a
> future patch.

Actually this patch is really low risk, while that one is high risk. I'd rather
we didn't have to roll out tons of patches if we need to revert 15159008. So I'm
going to land this one with the extra check.

[esprehn, 2013-05-22 00:32:16]

Committed patchset #3 manually as r150841 (presubmit successful).