Notification should not leak document.

Notification should not leak document.

The notification object was leaking the source document. The notification are
setPendingActivity() when show()-ed, but the code path to the finalize() method,
which calls unsetPendingActivity() internally, is corrupted. The dispatch*Event()
functions was skipped from WebNotification* impl, it called dispatchEvent directly.

This patch changes WebNotification to not call dispatchEvent() directly, but use
dispatch{Click,Close,Error,Show}Event() methods. This avoids code duplication.

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

[kouhei (in TOK), 2013-08-16 08:45:08]

abarth, haraken: PTAL

[haraken, 2013-08-16 14:58:03]

I'd like to delegate the review to Notification experts.

> a) Notification is made a ContextLifecycleObserver instead of ActiveDOMObject.
There is no reason to retain DOM tree.

I don't fully understand this part. The reason we're using ActiveDOMObject is
not that we want to retain a DOM tree but that we want to keep the Notification
wrapper alive as long as the Notification is being show()-n. (Consequently, the
DOM tree is retained through the Notification wrapper.) Thus, the problem is not
whether we should retain the DOM tree but whether we should retain the
Notification wrapper.

https://codereview.chromium.org/22909013/diff/3001/Source/web/WebNotification...
File Source/web/WebNotification.cpp (right):

https://codereview.chromium.org/22909013/diff/3001/Source/web/WebNotification...
Source/web/WebNotification.cpp:114: m_private->dispatchShowEvent();

Don't you need to check m_private->scriptExecutionContext() before calling
dispatchShowEvent() ? Before this CL, it was checked.

[kouhei (in TOK), 2013-08-20 00:58:56]

Thanks for your comments.

> I don't fully understand this part. The reason we're using ActiveDOMObject is
> not that we want to retain a DOM tree but that we want to keep the
Notification
> wrapper alive as long as the Notification is being show()-n. (Consequently,
the
> DOM tree is retained through the Notification wrapper.) Thus, the problem is
not
> whether we should retain the DOM tree but whether we should retain the
> Notification wrapper.

I agree that this might want a look from Notification expert. However, "git
log," didn't help here. Do you know who I should ask?
I don't want the document to be kept alive as there is no guarantee that user
would close it as soon as he/she notices it. Especially, notifications in mobile
tend to be ignored for long time if not ever.
According to the W3C spec, those notification callbacks are called
"best-effort," so disposing document before notification close should not be a
spec issue, but I'm not sure from UX point of view.

https://chromiumcodereview.appspot.com/22909013/diff/3001/Source/web/WebNotif...
File Source/web/WebNotification.cpp (right):

https://chromiumcodereview.appspot.com/22909013/diff/3001/Source/web/WebNotif...
Source/web/WebNotification.cpp:114: m_private->dispatchShowEvent();
On 2013/08/16 14:58:03, haraken wrote:
> 
> Don't you need to check m_private->scriptExecutionContext() before calling
> dispatchShowEvent() ? Before this CL, it was checked.

True, but I don't understand why it is needed. It does not cause assertion
error. Maybe it is provided as a fast path as no default handler for the events
exist?

[haraken, 2013-08-20 02:50:21]

johnnyg, atwilson: Would you mind taking a look at this change? We're not sure
if this change is OK or not in terms of the spec.

https://chromiumcodereview.appspot.com/22909013/diff/3001/Source/web/WebNotif...
File Source/web/WebNotification.cpp (right):

https://chromiumcodereview.appspot.com/22909013/diff/3001/Source/web/WebNotif...
Source/web/WebNotification.cpp:114: m_private->dispatchShowEvent();
On 2013/08/20 00:58:56, kouhei wrote:
> On 2013/08/16 14:58:03, haraken wrote:
> > 
> > Don't you need to check m_private->scriptExecutionContext() before calling
> > dispatchShowEvent() ? Before this CL, it was checked.
> 
> True, but I don't understand why it is needed. It does not cause assertion
> error. Maybe it is provided as a fast path as no default handler for the
events
> exist?

The check is needed because it's possible that notification events are
dispatched via Chromium APIs after the ScriptExecutionContext is destroyed.
Specifically, in the ActiveDOMObject world, it was possible that (1)
ActiveDOMObject::contextDestroyed() clears
ActiveDOMObject::m_scriptExecutionContext and then (2)
ActiveDOMObject::scriptExecutionContext() returns 0.

I think this check is still needed in your CL. Either way, I'd recommend you
remove the check in a follow-up CL even if you want to remove it.

[Andrew T Wilson (Slow), 2013-08-20 08:50:10]

On 2013/08/20 02:50:21, haraken wrote:
> johnnyg, atwilson: Would you mind taking a look at this change? We're not sure
> if this change is OK or not in terms of the spec.
> 

Sorry, I'm not a great candidate to review this CL as I don't recall the
intricacies of ActiveDOMObjects any longer, especially in conjunction with
Notifications.

Perhaps johnnyg knows more - you might also loop in dimich@ or one of the other
chrome-side notifications folks.

[johnnyg, 2013-08-20 09:21:06]

I'm not a great candidate either; it's been several years since I was
active in webkit code.


On Tue, Aug 20, 2013 at 1:50 AM, <atwilson@chromium.org> wrote:

> On 2013/08/20 02:50:21, haraken wrote:
>
>> johnnyg, atwilson: Would you mind taking a look at this change? We're not
>> sure
>> if this change is OK or not in terms of the spec.
>>
>
>
> Sorry, I'm not a great candidate to review this CL as I don't recall the
> intricacies of ActiveDOMObjects any longer, especially in conjunction with
> Notifications.
>
> Perhaps johnnyg knows more - you might also loop in dimich@ or one of the
> other
> chrome-side notifications folks.
>
>
https://chromiumcodereview.**appspot.com/22909013/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2013-08-20 09:24:39]

johnnyg, atwilson: Thanks!

dimich: Would you mind taking a look?

[abarth-chromium, 2013-08-20 19:24:09]

The Notification object needs to be an ActiveDOMObject.  The reason is that we
need to keep the wrapper for the Notification object alive unless we're sure
we're not going to fire any more events on the object.  At that point, the
Notificiation object shouldn't have any more pending activity and we can GC the
wrapper as usual.

[kouhei (in TOK), 2013-08-22 03:18:47]

Thanks for your comments. I updated the CL with just the leak fix, keeping
Notification subclass of ActiveDOMObject. PTAL.

https://codereview.chromium.org/22909013/diff/3001/Source/web/WebNotification...
File Source/web/WebNotification.cpp (right):

https://codereview.chromium.org/22909013/diff/3001/Source/web/WebNotification...
Source/web/WebNotification.cpp:114: m_private->dispatchShowEvent();
On 2013/08/20 02:50:21, haraken wrote:
> On 2013/08/20 00:58:56, kouhei wrote:
> > On 2013/08/16 14:58:03, haraken wrote:
> > > 
> > > Don't you need to check m_private->scriptExecutionContext() before calling
> > > dispatchShowEvent() ? Before this CL, it was checked.
> > 
> > True, but I don't understand why it is needed. It does not cause assertion
> > error. Maybe it is provided as a fast path as no default handler for the
> events
> > exist?
> 
> The check is needed because it's possible that notification events are
> dispatched via Chromium APIs after the ScriptExecutionContext is destroyed.
> Specifically, in the ActiveDOMObject world, it was possible that (1)
> ActiveDOMObject::contextDestroyed() clears
> ActiveDOMObject::m_scriptExecutionContext and then (2)
> ActiveDOMObject::scriptExecutionContext() returns 0.
> 
> I think this check is still needed in your CL. Either way, I'd recommend you
> remove the check in a follow-up CL even if you want to remove it.

I'll keep this check in this CL, and will do a separate CL for removing this.
Yes. It's possible to have scriptExecutionContext() == 0 when dispatchEvent(),
but I don't think it would be a problem, as implementation of dispatchEvent()
don't rely on it.

[haraken, 2013-08-22 03:30:44]

This CL looks safer. LGTM.

[kouhei (in TOK), 2013-08-22 03:41:12]

Thanks for the review!

[commit-bot: I haz the power, 2013-08-22 03:41:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/22909013/15001

[kouhei (in TOK), 2013-08-22 03:48:23]

abarth: Would you take a look? I updated the patch so it is kept as an
ActiveDOMObject now.

[abarth-chromium, 2013-08-22 18:46:06]

lgtm

[kouhei (in TOK), 2013-08-22 23:37:01]

Thanks for the review!

[commit-bot: I haz the power, 2013-08-22 23:37:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/22909013/15001

[commit-bot: I haz the power, 2013-08-23 01:28:38]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-08-23 01:28:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/22909013/36001

[commit-bot: I haz the power, 2013-08-23 03:23:37]

Change committed as 156610