Page MenuHomePhabricator
Create Task
Maniphest T146035

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

Description

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 )

Details

SubjectRepoBranchLines +/-
Merge mw.popups.experiment into mw.popups.coremediawiki/extensions/Popupswmf/1.28.0-wmf.20+96 -118
Merge mw.popups.experiment into mw.popups.coremediawiki/extensions/Popupsmaster+96 -118
Customize query in gerrit

Related Objects
Search...

Event Timeline

• Jhernandez created this task.Sep 19 2016, 2:43 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 19 2016, 2:43 PM
• Jhernandez added a parent task: T146017: Hovercards beta feature preference OFF not honored for logged in users when the experiment is enabled.Sep 19 2016, 3:27 PM
Jdlrobson subscribed.Sep 19 2016, 10:21 PM

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 merged a task: T146098: ext.popups.core should explicitly have dependency on ext.popups.experiment.
Jdlrobson added a subscriber: • Tbayer.
Jdlrobson added a comment.Sep 19 2016, 10:35 PM

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
phuedx subscribed.Sep 20 2016, 10:26 AM

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 = {}

phuedx added a subscriber: ovasileva.Sep 20 2016, 1:19 PM

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

• Jhernandez added a comment.Sep 20 2016, 4:21 PM

@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.

ovasileva triaged this task as High priority.Sep 20 2016, 4:49 PM
ovasileva edited projects, added Page-Previews (2016-17-Q2-Goal), Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names; removed Page-Previews.Sep 20 2016, 5:29 PM
MBinder_WMF subscribed.Sep 20 2016, 8:30 PM

@ovasileva Unplanned-Sprint-Work ? Needs an estimate at standup?

ovasileva added a comment.Sep 21 2016, 10:46 AM

@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
Restricted Application added subscribers: Jay8g, Luke081515, TerraCodes. · View Herald TranscriptSep 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 https://phabricator.wikimedia.org/T146035 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

ovasileva moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 21 2016, 11:11 AM
gerritbot subscribed.Sep 21 2016, 1:04 PM

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

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

gerritbot added a project: Patch-For-Review.Sep 21 2016, 1:04 PM
phuedx claimed this task.Sep 21 2016, 1:48 PM
phuedx moved this task from To Do to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.
MBinder_WMF added a project: Unplanned-Sprint-Work.Sep 21 2016, 3:58 PM
• jhobs moved this task from Code Review to -1 (Needs More Work) on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 21 2016, 4:04 PM
phuedx reassigned this task from phuedx to • jhobs.Sep 21 2016, 5:44 PM

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 - https://phabricator.wikimedia.org/T146035
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

phuedx moved this task from -1 (Needs More Work) to Doing on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 21 2016, 5:44 PM
Jdlrobson added a comment.Sep 21 2016, 7:38 PM

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.

• jhobs added a comment.Sep 21 2016, 7:46 PM

@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.

Jdlrobson added a comment.Sep 21 2016, 8:41 PM

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 moved this task from Doing to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 21 2016, 8:49 PM
• jhobs reassigned this task from • jhobs to Jdlrobson.Sep 21 2016, 9:07 PM
• 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.

gerritbot added a comment.Sep 21 2016, 10:31 PM

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

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

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-10-04_(1.28.0-wmf.21)).Sep 21 2016, 11:00 PM
Jdlrobson reassigned this task from Jdlrobson to phuedx.EditedSep 21 2016, 11:06 PM

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: https://wikitech.wikimedia.org/wiki/Deployments/Holding_the_train#What_happens_in_SWAT_while_the_train_is_on_hold.3F
4:04 PM for tonight
4:04 PM D— Dereckson nods.

Over to you!

Jdlrobson moved this task from Code Review to Doing on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 21 2016, 11:07 PM
• Jhernandez added a comment.Sep 22 2016, 10:24 AM

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.

gerritbot added a comment.Sep 22 2016, 10:31 AM

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

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

• Jhernandez added a comment.Sep 22 2016, 10:37 AM

I've QAd locally T146035#2658572 and it seems fine to me.

gerritbot added a comment.Sep 22 2016, 12:16 PM

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

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

Stashbot subscribed.Sep 22 2016, 12:31 PM

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)

phuedx added a subscriber: hashar.Sep 22 2016, 12:44 PM

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 moved this task from Doing to Ready for Signoff on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.
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?

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-09-20_(1.28.0-wmf.20)).Sep 22 2016, 1:00 PM
phuedx assigned this task to • Jhernandez.EditedSep 23 2016, 5:15 AM

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

• Tbayer mentioned this in T136746: Initiate Hovercards A/B Test 2: ru and it.Sep 23 2016, 8:11 AM
ovasileva closed this task as Resolved.Sep 23 2016, 3:49 PM
• Tbayer mentioned this in T146620: Schema:Popups losing pageLoaded events since September 8.Sep 26 2016, 9:33 PM
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