Fix crash on null ptr dereference in EventPath propagation

Fix crash on null ptr dereference in EventPath propagation

Usually ShadowRoot->host() is non-null, but a test case
revealed that during event propagation, non-null is
not guaranteed.

There are some conditions required to reproduce:
1. some synchronous events have to be queued
2. shadow host is removed during a prior event handler
3. a queued event is delivered to the orphaned shadow
   tree (including its shadow root, whose host is null)

This happens when a shadow host's ElementShadow object
is destructed.  If Oilpan is enabled, this doesn't happen
because the host's ElementShadow is not destructed
immediately.

This is from issue 1230503002 at patchset 1 (http://crrev.com/1230503002#ps1)
Modified to make the change effective only when Oilpan is
disabled.

BUG=507413
TEST=fast/event/event-fire-disconnected-shadow-dom-crash.html

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

[kochi, 2015-07-30 09:25:14]

kochi@chromium.org changed reviewers:
+ dtapuska@chromium.org, hayato@chromium.org

[kochi, 2015-07-30 09:25:48]

Hayato, could you review this?

[hayato, 2015-07-31 05:55:45]

On 2015/07/30 09:25:48, Takayoshi Kochi wrote:
> Hayato, could you review this?

I'm afraid that it's unclear why this layout test caused the crash. e.g. why we
need document.executeCommand("SelectAll");

It would be better to clarify that.

[kochi, 2015-07-31 06:06:54]

On 2015/07/31 05:55:45, hayato wrote:
> On 2015/07/30 09:25:48, Takayoshi Kochi wrote:
> > Hayato, could you review this?
> 
> I'm afraid that it's unclear why this layout test caused the crash. e.g. why
we
> need document.executeCommand("SelectAll");
> 
> It would be better to clarify that.

The context is on
https://code.google.com/p/chromium/issues/detail?id=507413#c8
isn't it?

Shall I include the reference in the test?

And do you prefer rename the test, as the "event-fire-disconnected-shadow-dom"
doesn't really express what it tests, rather it is a reproduction test
for the reported crash?

[hayato, 2015-08-03 01:30:59]

On 2015/07/31 06:06:54, Takayoshi Kochi wrote:
> On 2015/07/31 05:55:45, hayato wrote:
> > On 2015/07/30 09:25:48, Takayoshi Kochi wrote:
> > > Hayato, could you review this?
> > 
> > I'm afraid that it's unclear why this layout test caused the crash. e.g. why
> we
> > need document.executeCommand("SelectAll");
> > 
> > It would be better to clarify that.
> 
> The context is on
> https://code.google.com/p/chromium/issues/detail?id=507413#c8
> isn't it?
> 
> Shall I include the reference in the test?
> 
> And do you prefer rename the test, as the "event-fire-disconnected-shadow-dom"
> doesn't really express what it tests, rather it is a reproduction test
> for the reported crash?

I think the description of CL should mention:

- When and why shadowRoot.shadowHost() becomes null more clearly. I'm afraid
that most readers can't read it from the description, the test, or the bug.
- I'm afraid that blink developers would wonder when they should check
shadowRoot.shadowHost()'s null-ness. They don't have a clear answer from this
CL, I guess.

[kochi, 2015-08-03 04:54:21]

On 2015/08/03 01:30:59, hayato wrote:
> On 2015/07/31 06:06:54, Takayoshi Kochi wrote:
> > On 2015/07/31 05:55:45, hayato wrote:
> > > On 2015/07/30 09:25:48, Takayoshi Kochi wrote:
> > > > Hayato, could you review this?
> > > 
> > > I'm afraid that it's unclear why this layout test caused the crash. e.g.
why
> > we
> > > need document.executeCommand("SelectAll");
> > > 
> > > It would be better to clarify that.
> > 
> > The context is on
> > https://code.google.com/p/chromium/issues/detail?id=507413#c8
> > isn't it?
> > 
> > Shall I include the reference in the test?
> > 
> > And do you prefer rename the test, as the
"event-fire-disconnected-shadow-dom"
> > doesn't really express what it tests, rather it is a reproduction test
> > for the reported crash?
> 
> I think the description of CL should mention:
> 
> - When and why shadowRoot.shadowHost() becomes null more clearly. I'm afraid
> that most readers can't read it from the description, the test, or the bug.
> - I'm afraid that blink developers would wonder when they should check
> shadowRoot.shadowHost()'s null-ness. They don't have a clear answer from this
> CL, I guess.

Added more comments and updated the CL description.
Renamed the test as well to be distinguishable with other similarly named
tests in the same directory (event-fire-disconnected-bubbling-*) which 
tests normal path, not crash tests.

The Node::shadowHost() API used to return nullptr when it is not in a shadow
tree (according to the comment), but it is no longer true in such a event
handlers.

This means potentially other call sites of ShadowRoot::host() or
Node::shadowHost() may be affected by this, which this CL does not fix.
I'll take more closer look at other sites, but this CL independently
fixes this specific case.

[hayato, 2015-08-03 05:49:25]

Thanks. The description looks much better!

https://codereview.chromium.org/1265573003/diff/80001/LayoutTests/fast/events...
File LayoutTests/fast/events/event-fire-disconnected-shadow-dom-crash.html
(right):

https://codereview.chromium.org/1265573003/diff/80001/LayoutTests/fast/events...
LayoutTests/fast/events/event-fire-disconnected-shadow-dom-crash.html:1:
<!DOCTYPE html>
Could we minimize this test further?
I can't read why we need <option> twice and why we need scopedCall() to
reproduce a crash.

It would be helpful to leave your thought about this HTML.

It it's difficult to know what would happen in background, it might be useful to
record that which shadow host was removed and whose shadow root's host would be
null in this test.

Couldn't we reproduce a crash by dispatching a synthetic event?
From the description, this sounds possible. Removing a shadow host and
dispatching an event on shadow root, by keeping a reference to the shadow host,
aren't enough?

[kochi, 2015-08-03 11:19:48]

On 2015/08/03 05:49:25, hayato wrote:
> Thanks. The description looks much better!
> 
>
https://codereview.chromium.org/1265573003/diff/80001/LayoutTests/fast/events...
> File LayoutTests/fast/events/event-fire-disconnected-shadow-dom-crash.html
> (right):
> 
>
https://codereview.chromium.org/1265573003/diff/80001/LayoutTests/fast/events...
> LayoutTests/fast/events/event-fire-disconnected-shadow-dom-crash.html:1:
> <!DOCTYPE html>
> Could we minimize this test further?
> I can't read why we need <option> twice and why we need scopedCall() to
> reproduce a crash.
> 
> It would be helpful to leave your thought about this HTML.
> 
> It it's difficult to know what would happen in background, it might be useful
to
> record that which shadow host was removed and whose shadow root's host would
be
> null in this test.
> 
> Couldn't we reproduce a crash by dispatching a synthetic event?
> From the description, this sounds possible. Removing a shadow host and
> dispatching an event on shadow root, by keeping a reference to the shadow
host,
> aren't enough?

further minimized the test case and documented the details of what happens.

The inserted HTML "<option>A<a><option>C</option></a></option>" is very tricky.

I'm not sure the details of HTML parser and to which element/node that
DOMNodeInserted event is delivered, but the order of the queued events
and removed nodes play important role in reproducing the crash.

[hayato, 2015-08-04 04:35:15]

On 2015/08/03 11:19:48, Takayoshi Kochi wrote:
> On 2015/08/03 05:49:25, hayato wrote:
> > Thanks. The description looks much better!
> > 
> >
>
https://codereview.chromium.org/1265573003/diff/80001/LayoutTests/fast/events...
> > File LayoutTests/fast/events/event-fire-disconnected-shadow-dom-crash.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/1265573003/diff/80001/LayoutTests/fast/events...
> > LayoutTests/fast/events/event-fire-disconnected-shadow-dom-crash.html:1:
> > <!DOCTYPE html>
> > Could we minimize this test further?
> > I can't read why we need <option> twice and why we need scopedCall() to
> > reproduce a crash.
> > 
> > It would be helpful to leave your thought about this HTML.
> > 
> > It it's difficult to know what would happen in background, it might be
useful
> to
> > record that which shadow host was removed and whose shadow root's host would
> be
> > null in this test.
> > 
> > Couldn't we reproduce a crash by dispatching a synthetic event?
> > From the description, this sounds possible. Removing a shadow host and
> > dispatching an event on shadow root, by keeping a reference to the shadow
> host,
> > aren't enough?
> 
> further minimized the test case and documented the details of what happens.
> 
> The inserted HTML "<option>A<a><option>C</option></a></option>" is very
tricky.
> 
> I'm not sure the details of HTML parser and to which element/node that
> DOMNodeInserted event is delivered, but the order of the queued events
> and removed nodes play important role in reproducing the crash.

Thanks. The comment in the test would be helpful to understand the background.

lgtm

[kochi, 2015-08-04 04:36:47]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-08-04 04:36:58]

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

[commit-bot: I haz the power, 2015-08-04 05:50:42]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199940