phabricator.wikimedia.org

⚓ T107979 [Regression pre-wmf18] VisualEditor doesn't load, throwing "Uncaught TypeError: Cannot read property 'requestPageData' of undefined"

  • ️Wed Aug 05 2015

[Regression pre-wmf18] VisualEditor doesn't load, throwing "Uncaught TypeError: Cannot read property 'requestPageData' of undefined"

Closed, ResolvedPublic8 Estimated Story Points

/static/master/extensions/VisualEditor/modules/ve-mw/init/targets/ve.init.mw.DesktopArticleTarget.init.js:156 Uncaught TypeError: Cannot read property 'requestPageData' of undefined

mw.libs.ve.targetLoader is defined in ext.visualEditor.targetLoader, of which an mw.loader.using wrapper encloses this line, so… help.

Event Timeline

Comment Actions

The problem is that there is a race condition between two paths:

  1. DesktopArticleTarget.init loading synchronously in the top queue, and (once ready) it subsequently loads ext.visualEditor.targetLoader asynchronously.
  2. ext.visualEditor.targetLoader asynchronously loading in the bottom queue.

Commit 812cb9aa2 acknowledges that this race condition happens sometimes, but did not address it correctly.

DesktopArticleTarget.init is where the mw.libs.ve object is primarily created. The TargetLoader extends this object with an additional .targetLoader property. Since TargetLoader can (and should) arrive independently it means the host object may be missing at that time.

812cb9aa2 added tolerance in TargetLoader by lazy-initializing that object. However that only fixes one of the two exceptions. It prevented TargetLoader from failing for mw.lib.ve being undefined. But it does not prevent DesktopArticleTarget.init from failing on mw.lib.ve.targetLoader (as we're seeing right now) because DesktopArticleTarget.init will still (re-)create the host object as usual and thus blow away the TargetLoader's property.

In the past, the proper solution would've been to add a dependency on DesktopArticleTarget.init for ext.visualEditor.targetLoader so that execution unfolds in the right order (regardless of load order). However since TargetLoader is used by both Desktop and Mobile targets, a dependency is not an option.

In the short-term I'd recommend we add tolerance, similar to 812cb9aa2, in DesktopArticleTarget.init. E.g. lazy-create the object and use $.extend or property assignments for the methods instead of assigning the object as whole. That way pre-existing properties (such as TargetLoader) are preserved.

In the medium-term I'd recommend we restore the fact that mw.libs.ve is owned by a single common module (instead of tolerated from two sides) and merely extended by things like Desktop and TargetLoader. Especially because right now some "public methods" in mw.libs.ve are missing on Mobile because its target doesn't define them (it only loads TargetLoader). E.g. mw.libs.ve.addPlugin and mw.libs.ve.isAvailable are missing on mobile.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits