Gracefully handle a disconnected popup menu client on disposing open popup.

Gracefully handle a disconnected popup menu client on disposing open popup.

If the client of a PopupMenu disconnects itself before destructing a
currently open popup menu (and its page), handle the fact that the client
is not reachable for notifications.

R=keishi,haraken
BUG=457383
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189983

[sof, 2015-02-11 09:50:35]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, keishi@chromium.org, tkent@chromium.org

[sof, 2015-02-11 09:50:36]

Please take a look.

(Artificial-looking testcase synthesized from clusterfuzz.)

[keishi, 2015-02-11 11:44:08]

Thanks! LGTM

https://codereview.chromium.org/920433002/diff/1/LayoutTests/fast/forms/selec...
File LayoutTests/fast/forms/select-popup-close-no-crash.html (right):

https://codereview.chromium.org/920433002/diff/1/LayoutTests/fast/forms/selec...
LayoutTests/fast/forms/select-popup-close-no-crash.html:20: function
openPicker(menu) {
We have this function in fast/forms/resources/picker-common.js
I think this will work.
openPicker(document.getElementById('menu'), function () {
    document.getElementById('form1').submit();
});

[sof, 2015-02-11 11:56:08]

New patchsets have been uploaded after l-g-t-m from keishi@chromium.org

[sof, 2015-02-11 11:58:50]

On 2015/02/11 11:44:08, keishi wrote:
> Thanks! LGTM
> 
>
https://codereview.chromium.org/920433002/diff/1/LayoutTests/fast/forms/selec...
> File LayoutTests/fast/forms/select-popup-close-no-crash.html (right):
> 
>
https://codereview.chromium.org/920433002/diff/1/LayoutTests/fast/forms/selec...
> LayoutTests/fast/forms/select-popup-close-no-crash.html:20: function
> openPicker(menu) {
> We have this function in fast/forms/resources/picker-common.js
> I think this will work.
> openPicker(document.getElementById('menu'), function () {
>     document.getElementById('form1').submit();
> });

Thanks, worked perfectly.

(will wait on Source/web approval.)

[haraken, 2015-02-11 14:13:30]

LGTM

[sof, 2015-02-11 14:19:08]

On 2015/02/11 14:13:30, haraken wrote:
> LGTM

Thanks (doubly so, just realized that it is a national holiday in Japan today.)

[sof, 2015-02-11 14:19:28]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-11 14:20:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/920433002/20001

[sof, 2015-02-11 15:43:25]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2015-02-11 15:45:15]

On 2015/02/11 14:20:19, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/920433002/20001

The added test isn't stable on mac, will have to address.

[keishi, 2015-02-11 16:06:15]

On 2015/02/11 15:45:15, sof wrote:
> On 2015/02/11 14:20:19, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/920433002/20001
> 
> The added test isn't stable on mac, will have to address.

Sorry. PopupMenuImpl isn't used on Mac so openPicker doesn't work and the
openPicker callback isn't called.
openPicker takes an error callback as the third argument so you can use that.
Something like:

 openPicker(document.getElementById('menu'), function () { document.getElemen
    tById('form1').submit(); }, function() { console.log('popup didn\'t open'):
testRunner.notifyDone(); });

[sof, 2015-02-11 19:43:30]

New patchsets have been uploaded after l-g-t-m from haraken@chromium.org

[sof, 2015-02-11 19:45:54]

On 2015/02/11 16:06:15, keishi wrote:
> On 2015/02/11 15:45:15, sof wrote:
> > On 2015/02/11 14:20:19, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/920433002/20001
> > 
> > The added test isn't stable on mac, will have to address.
> 
> Sorry. PopupMenuImpl isn't used on Mac so openPicker doesn't work and the
> openPicker callback isn't called.
> openPicker takes an error callback as the third argument so you can use that.
> Something like:
> 
>  openPicker(document.getElementById('menu'), function () { document.getElemen
>     tById('form1').submit(); }, function() { console.log('popup didn\'t
open'):
> testRunner.notifyDone(); });

Thanks for the pointer, that saved me some time locating the source of the
problem.

[sof, 2015-02-11 19:46:12]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-11 19:48:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/920433002/40001

[commit-bot: I haz the power, 2015-02-11 21:44:53]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189983