Page MenuHomePhabricator

[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/ Uncaught TypeError: Cannot read property 'requestPageData' of undefined is defined in ext.visualEditor.targetLoader, of which an mw.loader.using wrapper encloses this line, so… help.

Event Timeline

Jdforrester-WMF raised the priority of this task from to Unbreak Now!.
Jdforrester-WMF updated the task description. (Show Details)
Jdforrester-WMF moved this task to TR0: Interrupt on the VisualEditor board.

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 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 being undefined. But it does not prevent DesktopArticleTarget.init from failing on (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 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 are missing on Mobile because its target doesn't define them (it only loads TargetLoader). E.g. and are missing on mobile.

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

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

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

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