Page MenuHomePhabricator

OOjs UI: Trying to close a window before it's ready causes a JS error
Closed, ResolvedPublic

Description

An easy way to reproduce this is to add this.manager.closeWindow( this ); to the setup process (or ready process, probably) of any window.

closeWindow caches the value of this.opened, and it later assumes that this value will be non-null (it calls .resolve() on it), but this.opened is initialized by openWindow only when the window is ready, while currentWindow is set earlier. So it's possible for closeWindow to go into its "there is something to close" code path (because currentWindow is set) while this.opened is still null, and then explore later while trying to call opened.resolve().

Before you tell me that having a window that closes itself from its own setup/ready is stupid: yes, it is, but 1) something else might try to close it at exactly the right moment, and 2) self-closing from setup/ready actually happens in IE right now (that's what https://gerrit.wikimedia.org/r/155181 fixes) because setup/ready moves the focus which causes the context to close the inspector.

I don't know exactly how best to fix this. Maybe manager.opened should be initialized earlier, at the same time as currentWindow? We only expose it later when we resolve opening, but we could create it early so we can resolve it early.


Version: unspecified
Severity: normal

Details

Reference
bz69918

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:36 AM
bzimport added a project: OOUI.
bzimport set Reference to bz69918.

Change 166613 had a related patch set uploaded by Bartosz Dziewoński:
WindowManager: Wait for window to open before trying to close it

https://gerrit.wikimedia.org/r/166613

Change 166613 merged by jenkins-bot:
WindowManager: Wait for window to open before trying to close it

https://gerrit.wikimedia.org/r/166613