Page MenuHomePhabricator

Popups ResourceLoader modules are not declaring their dependencies properly
Closed, ResolvedPublic1 Estimated Story Points


TypeError: Cannot read property 'isUserInCondition' of undefined(…)

load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:176 TypeError: Cannot read property 'isUserInCondition' of undefined TypeError: Cannot read property 'isUserInCondition' of undefined(…)log @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:176handler @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:153fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:45fireWith @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:46fire @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:46track @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:153runScript @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:161checkCssHandles @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:161execute @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:162implement @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:168(anonymous function) @ VM851:1(anonymous function) @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:4globalEval @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:4iterate @ load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=0oyf810:166

Very hard to reproduce, but I've seen it a few times.

Seems like mw.popups.experiment is sometimes not defined when resources/ext.popups.core.js#mw.popups.getEnabledState is invoked. Looking at extension.json:

  • ext.popups.core has the script resources/ext.popups.core.js which calls mw.popups.experiment.isUserInCondition
  • ext.popups.experiment
    • has the script resources/ext.popups.experiment.js that defines mw.popups.experiment.isUserInCondition
    • has ext.popups.core in its dependencies
  • ext.popups.targets.desktopTarget which lists first ext.popups.core and then ext.popups.experiment in its dependencies
  • ext.popups.schemaPopups.utils which lists first ext.popups.experiment and then ext.popups.core in its dependencies
    • gets loaded from a hook

Looking at the code, if the ext.popups.core module is loaded before mw.popups.experiment it could cause the above error which might break the feature and interfere with event logging as the property popupEnabled uses mw.popups.getEnabledState() which uses isUserInCondition. If this required value was undefined this would lead to dropped events. (cc @Tbayer )

Event Timeline

Hah I just found the same problem and saw your card after creating it. Lemme flesh out and merge the problems.

Jdlrobson renamed this task from Non-deterministic exception thrown in console to Non-deterministic exception thrown in console (ext.popups.core should explicitly have dependency on ext.popups.experiment).Sep 19 2016, 10:21 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Tbayer.

Similarly I'm seeing the following issue:

Uncaught TypeError: mw.popups.render.abortCurrentRequest is not a function

This is caused by ext.popups.desktopnot declaring ext.popups.renderer as a dependency. Sigh.

Joaquin any chance of your linting tool helping here?

Jdlrobson renamed this task from Non-deterministic exception thrown in console (ext.popups.core should explicitly have dependency on ext.popups.experiment) to Popups ResourceLoader modules are not declaring their dependencies properly.Sep 19 2016, 10:35 PM

From IRC:

<joakino> from my headbanging against the wall, it is because core defines mw.popups = {} and experiment uses it to define its own subnamespace mw.popups.experiment = {}

@ovasileva: @Jdlrobson and @Jhernandez can provide more detail but AFAICT these intermittent errors stop hovercards working.

@ovasileva This is high priority given the push on hovercards and the upcoming browser tests.

I've put it as the blocker of the blocker of the test but feel free to move it around.

@MBinder_WMF - yes, we do. Also, since we want to deploy tomorrow, I'm upping the priority.

ovasileva raised the priority of this task from High to Unbreak Now!.Sep 21 2016, 10:46 AM
ovasileva set the point value for this task to 1.Sep 21 2016, 11:09 AM

added story points as per:

joakino , phuedx - sorry to bug but you're the only ones awake - we didn't estimate yesterday and it's still in needs analysis - do you think that puts us behind schedule
12:51 PM P<phuedx> Sam Smith this isn't a dream?
12:52 PM joakino, olliv: 1 maybe a 2
12:52 PM probably a 1
12:52 PM it's an audit of the configuration file
12:54 PM O<olliv> Olga Vasileva phuedx, thank you! if joakino agrees, then I'll put the estimate and throw it into todo
12:55 PM these days, hovercards are all my bad dreams XD
1:08 PM → stephanebisson joined (~stephaneb@wikimedia/sbisson-wmf)
1:08 PM J<joakino> Joaquin do so olliv
1:09 PM im out till this afternoon

Change 311977 had a related patch set uploaded (by Phuedx):

From IRC:

18:21:29 <phuedx> jhobs: i'm gonna be out in 10 minutes
18:23:07 <phuedx> can i assign you to T146035?
18:23:08 <stashbot> T146035: Popups ResourceLoader modules are not declaring their dependencies properly -
18:23:14 <phuedx> i think you're right
18:23:43 <phuedx> i'm not too keen on the experiment stuff being part of core but, as you say, it'll be removed later
18:24:18 <jhobs> phuedx: yeah that's fine. Would you be fine with SWAT deploying it during European SWAT tomorrow so that way you get a chance to review it before deploy?
18:26:17 <jhobs> as in, I'd write it today and schedule you as the person on-call tomorrow
18:26:58 <jhobs> otherwise, I can do it during SF Morning SWAT tomorrow, but I'd rather get it in sooner rather than later
18:30:14 <phuedx> sure, put me in as call

I left some comments on the ticket. I can also swat this tonight @jhobs if you are able to get it to a state you are satisified with.

@Jdlrobson Thanks for the info on jsduck. Regarding implementation approach, I'm going to make the changes discussed in gerrit/IRC now and hopefully we can SWAT tonight. I'm also okay with the current state of the patch in terms of a hotfix, so why don't we schedule it for evening SWAT and if I can't get the changes in time for some reason (or rather, reviewed fast enough), we can just SWAT it in its current state.

Sounds like a plan. Let me know by 3pm PST if you want me to SWAT so I can add it to the calendar.

jhobs subscribed.

Over to @Jdlrobson for SWAT, per IRC:

4:50:39 PM <jhobs> jdlrobson: I just uploaded the change to Sam's patch regarding popups dependencies. If it looks good to you, let me know and I'll sign you up for evening SWAT tonight
5:01:14 PM <jhobs> jdlrobson: as I have to run now and I take it you haven't had a chance to look at it yet, I'll leave the SWAT scheduling to you. I'll ping on the task too just to be safe
5:01:27 PM <jdlrobson> jhobs: did you switch your -1 to a +1 ?
5:01:37 PM <jdlrobson> i assume you mean swat Sam's patch as is?
5:01:45 PM <jhobs> jdlrobson: no, I uploaded a new PS
5:02:13 PM <jdlrobson> okay let me take a quick look
5:04:38 PM <jdlrobson> ill give it a pass at review now. If i dont find any issues i'll swat and if i do find any issues i'll comment on task and leave you/sam to swat.
5:04:54 PM <jdlrobson> ill probably get a coffee first though as im waning :)
5:05:00 PM <jhobs> jdlrobson: sounds good. See you tomorrow! o/

If there are any issues that block deployment tonight, just assign it back to me or @phuedx.

Change 311977 merged by jenkins-bot:
Merge mw.popups.experiment into mw.popups.core

I'm gonna have to push this back to you guys I'm afraid.

greg-g> Greg Grossmeier Dereckson: since we're still officially in a held state re the train, I'd like to follow this:
4:04 PM for tonight
4:04 PM D— Dereckson nods.

Over to you!

Train is on hold because of performance regression: see T144644

Apparently deploy to group 0 of wmf20 should happen today but it is not confirmed and the schedule is outdated.

Change 312226 had a related patch set uploaded (by Phuedx):
Merge mw.popups.experiment into mw.popups.core

Change 312226 merged by jenkins-bot:
Merge mw.popups.experiment into mw.popups.core

Mentioned in SAL (#wikimedia-operations) [2016-09-22T12:31:38Z] <hashar@tin> Synchronized php-1.28.0-wmf.20/extensions/Popups: Merge mw.popups.experiment into mw.popups.core T146035 (duration: 00m 49s)

rEPOP0b2961c31879: Merge mw.popups.experiment into mw.popups.core was cherrypicked to 1.28.0-wmf.20 and deployed to the group 0 wikis. It'll be deployed to group 1 and 2 wikis (the Wikipedias) later today.

Thanks for all your hard work @hashar!

phuedx removed phuedx as the assignee of this task.EditedSep 22 2016, 12:44 PM
phuedx added a subscriber: bmansurov.

@bmansurov, @jhobs, @Jdlrobson: If and when the train rolls out today, could you take a look at Popups on the Wikipedias and sign this off?

@Jhernandez: I figured that you'd want to take a look but this can probably be signed off immediately given T136746#2661022.