Page MenuHomePhabricator

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

Description

/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

Jdforrester-WMF updated the task description. (Show Details)
Jdforrester-WMF raised the priority of this task from to Unbreak Now!.
Jdforrester-WMF moved this task to TR0: Interrupt on the VisualEditor board.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 5 2015, 2:24 AM
Krinkle added a comment.EditedAug 5 2015, 7:06 PM

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.

Change 229463 had a related patch set uploaded (by Catrope):
DesktopArticleTarget.init: Don't overwrite mw.libs.ve

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

Change 229463 merged by jenkins-bot:
DesktopArticleTarget.init: Don't overwrite mw.libs.ve

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

Jdforrester-WMF closed this task as Resolved.Aug 5 2015, 11:02 PM
Jdforrester-WMF assigned this task to Catrope.
Jdforrester-WMF removed a project: Patch-For-Review.
Jdforrester-WMF set Security to None.
Jdforrester-WMF edited a custom field.

Change 229976 had a related patch set uploaded (by Krinkle):
DesktopArticleTarget.init: Don't overwrite mw.libs.ve

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

Change 229976 merged by jenkins-bot:
DesktopArticleTarget.init: Don't overwrite mw.libs.ve

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