Protect an element while removing an attribute.

Protect an element while removing an attribute.

It's possible for attribute removal to do non-trivial work (e.g., in the case
of removing an iframe's src attr, which navigates the iframe to about:blank)

BUG=382279
TEST=http/tests/misc/adopt-iframe-src-attr-after-remove.html

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

[sof, 2014-06-20 18:28:07]

https://codereview.chromium.org/341273006/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/341273006/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2864: RefPtr<Element> protect(this);
Make it Oilpan friendly as "RefPtrWillBeRawPtr<Element> ..."

[abarth-chromium, 2014-06-20 18:38:25]

+esprehn, who probably is the best reviewer here.

[esprehn, 2014-06-20 20:01:31]

I don't understand this test case, the script should be keeping the frame alive
with the "i" variable in the setTimeout closure where you remove the attribute.

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
File LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html
(right):

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html:1: We pass
if we don't crash under ASAN.
Missing doctype.

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html:2: <iframe
id=i src="../navigation/resources/blank.txt"></iframe>
Can we use iframe instead of "i"?

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html:11:
i.parentNode.removeChild(i);
i.remove();

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html:17:
document.adoptNode(i.attributes["src"]);
Just use removeAttribute? I don't know why you're adopting the Attr node like
this.

https://codereview.chromium.org/341273006/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/341273006/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2864: RefPtr<Element> protect(this);
Does the RefPtr need to be higher up, what about the caller of this function?
They're left with a free |this|. I guess we call didRemoveAttribute last in the
chain, but that's pretty brittle.

Normally we put these at the top of the call stacks, so inside
::removeAttribute.

[Nate Chapin, 2014-06-20 20:17:43]

My understanding of the sequence of the test:

* DOMContentLoaded fires, the src attribute is removed. A synchronous navigation
to about:blank begins.
* The iframe's in-progress provisional load is stopped in
NavigationScheduler::schedule. The load event fires synchronously.
* The load event removes the iframe and forces a gc. I believe, at this point,
all uses of i are out of scope, and therefore i is gc'ed. I'm a little hazy on
the finer point of gc timing, but it's definitely reliably reclaimed.

cc'ed you to the bug in case you want to see the clusterfuzz stacks.

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
File LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html
(right):

https://codereview.chromium.org/341273006/diff/1/LayoutTests/http/tests/misc/...
LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html:17:
document.adoptNode(i.attributes["src"]);
On 2014/06/20 20:01:30, esprehn wrote:
> Just use removeAttribute? I don't know why you're adopting the Attr node like
> this.

I was building this off of a clusterfuzz testcase, so I left it minimally
modified. I suspect removeAttribute would suffice.

https://codereview.chromium.org/341273006/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/341273006/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2864: RefPtr<Element> protect(this);
On 2014/06/20 20:01:31, esprehn wrote:
> Does the RefPtr need to be higher up, what about the caller of this function?
> They're left with a free |this|. I guess we call didRemoveAttribute last in
the
> chain, but that's pretty brittle.
> 
> Normally we put these at the top of the call stacks, so inside
> ::removeAttribute.

I perhaps over-optimized that, will move them to the various callers.

[esprehn, 2014-06-20 20:51:58]

On 2014/06/20 at 20:17:43, japhet wrote:
> My understanding of the sequence of the test:
> 
> * DOMContentLoaded fires, the src attribute is removed. A synchronous
navigation to about:blank begins.
> * The iframe's in-progress provisional load is stopped in
NavigationScheduler::schedule. The load event fires synchronously.
> * The load event removes the iframe and forces a gc. I believe, at this point,
all uses of i are out of scope, and therefore i is gc'ed. I'm a little hazy on
the finer point of gc timing, but it's definitely reliably reclaimed.
> 

That sounds like the real bug then, the load event should never fire
synchronously down there.

[esprehn, 2014-06-20 21:08:09]

On 2014/06/20 at 20:51:58, esprehn wrote:
> On 2014/06/20 at 20:17:43, japhet wrote:
> > My understanding of the sequence of the test:
> > 
> > * DOMContentLoaded fires, the src attribute is removed. A synchronous
navigation to about:blank begins.
> > * The iframe's in-progress provisional load is stopped in
NavigationScheduler::schedule. The load event fires synchronously.
> > * The load event removes the iframe and forces a gc. I believe, at this
point, all uses of i are out of scope, and therefore i is gc'ed. I'm a little
hazy on the finer point of gc timing, but it's definitely reliably reclaimed.
> > 
> 
> That sounds like the real bug then, the load event should never fire
synchronously down there.

Also, that still doesn't seem right. The local variable "i" inside the code
that's removing the attribute should be keeping the <iframe> itself alive.

[esprehn, 2014-06-20 21:11:35]

On 2014/06/20 at 21:08:09, esprehn wrote:
> On 2014/06/20 at 20:51:58, esprehn wrote:
> > On 2014/06/20 at 20:17:43, japhet wrote:
> > > My understanding of the sequence of the test:
> > > 
> > > * DOMContentLoaded fires, the src attribute is removed. A synchronous
navigation to about:blank begins.
> > > * The iframe's in-progress provisional load is stopped in
NavigationScheduler::schedule. The load event fires synchronously.
> > > * The load event removes the iframe and forces a gc. I believe, at this
point, all uses of i are out of scope, and therefore i is gc'ed. I'm a little
hazy on the finer point of gc timing, but it's definitely reliably reclaimed.
> > > 
> > 
> > That sounds like the real bug then, the load event should never fire
synchronously down there.
> 
> Also, that still doesn't seem right. The local variable "i" inside the code
that's removing the attribute should be keeping the <iframe> itself alive.

abarth@ clarified this for me. This test is accessing the iframe by use of the
global scope polluter which doesn't actually keep things alive. :(

So indeed the only thing to fix is that the load event should fire async.

[esprehn, 2014-06-20 21:24:45]

https://codereview.chromium.org/341273006/diff/20001/LayoutTests/http/tests/m...
File LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html
(right):

https://codereview.chromium.org/341273006/diff/20001/LayoutTests/http/tests/m...
LayoutTests/http/tests/misc/adopt-iframe-src-attr-after-remove.html:17:
document.adoptNode(i.attributes["src"]);
It abarth explained to me the reason for the adoptNode. The "i" variable is gone
since we've called down into adoptNode to remove the Attr node and for some
reason the Attr is not keeping the owner element alive.

So the two bugs here seem to be:

1) Attr should keep the owner element alive.
2) onload should never fire synchronously.

[abarth-chromium, 2014-06-20 22:44:38]

LGTM

I talked this over with Nate.  We're not sure whether this covers all the cases,
but it seems like a reasonable change and won't have any negative effects
because we're in the ATTRIBUTE_NODE case of adoptNode, which is very rare.

[abarth-chromium, 2014-06-20 22:45:14]

On 2014/06/20 at 21:24:45, esprehn wrote:
> 1) Attr should keep the owner element alive.

Nate and I think that will cause a reference cycle.

> 2) onload should never fire synchronously.

We'd love to do that, but that's quite complex.

[Nate Chapin, 2014-06-20 22:47:51]

The CQ bit was checked by japhet@chromium.org

[esprehn, 2014-06-20 22:57:44]

On 2014/06/20 at 22:45:14, abarth wrote:
> On 2014/06/20 at 21:24:45, esprehn wrote:
> > 1) Attr should keep the owner element alive.
> 
> Nate and I think that will cause a reference cycle.

okay, lgtm them.

> 
> > 2) onload should never fire synchronously.
> 
> We'd love to do that, but that's quite complex.

Is there a reason we can't just setTimeout(0) it?

[commit-bot: I haz the power, 2014-06-20 23:00:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/341273006/40001

[commit-bot: I haz the power, 2014-06-21 01:12:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-06-21 01:43:47]

Change committed as 176668

[abarth-chromium, 2014-06-21 05:43:37]

On 2014/06/20 at 22:57:44, esprehn wrote:
> > > 2) onload should never fire synchronously.
> > 
> > We'd love to do that, but that's quite complex.
> 
> Is there a reason we can't just setTimeout(0) it?

Yes.  There's a specific order in which you need to drive the various lifecycle
events.  Given that we can't move them all to the event look (e.g., unload needs
to happen synchronously in some call stacks), we need to be careful about how we
schedule everything.

It's a doable project, but it's not as simple as just adding a timer.

[abarth-chromium, 2014-06-21 05:43:48]

event loop

[falken, 2014-07-14 09:52:18]

On 2014/06/21 05:43:48, abarth wrote:
> event loop

It looks like this test has had flaky timeouts since it was added. Would you be
willing to revert or fix the test?

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...