(At very least we should make this code more defensive and possibly log a custom error using mw.errorLogger.logError:

mw.errorLogger.logError(new Error(`T360781: ${typeof $container`), 'error.web')
`

Change #1014003 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/MinervaNeue@master] Guard against undefined $container element in initMobile.js

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

Change #1014003 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Guard against undefined $container element in initMobile.js

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

Change #1013645 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/MinervaNeue@wmf/1.42.0-wmf.23] Guard against undefined $container element in initMobile.js

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

Change #1013645 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@wmf/1.42.0-wmf.23] Guard against undefined $container element in initMobile.js

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

Mentioned in SAL (#wikimedia-operations) [2024-03-25T20:25:03Z] <cjming@deploy1002> Started scap: Backport for [[gerrit:1013645|Guard against undefined $container element in initMobile.js (T360781)]]

Mentioned in SAL (#wikimedia-operations) [2024-03-25T20:27:31Z] <cjming@deploy1002> cjming and jdrewniak: Backport for [[gerrit:1013645|Guard against undefined $container element in initMobile.js (T360781)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-03-25T20:42:48Z] <cjming@deploy1002> Finished scap: Backport for [[gerrit:1013645|Guard against undefined $container element in initMobile.js (T360781)]] (duration: 17m 45s)

Change #1014548 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/MinervaNeue@master] Add custom error logging for T360781

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

It looks like the error suppression was ineffective, which means that checking for $container && $container[ 0 ] was insufficient.
The following error: TypeError: Cannot read properties of undefined (reading 'addEventListener') is still thrown when attaching an eventHandler, i.e $container[ 0 ].addEventHandler(...)
Which means that maybe $container it's not a typeof HTMLElement.

This is unexpected, maybe related to gadgets? Some of the stacktraces mention XTools, e.g.

url: https://es.m.wikipedia.org/wiki/Shrek

at initMediaViewer  https://es.m.wikipedia.org/w/load.php?lang=es&modules=skins.minerva.scripts&skin=minerva&version=15hi7:7:317
at eval  https://es.m.wikipedia.org/w/load.php?lang=es&modules=skins.minerva.scripts&skin=minerva&version=15hi7:12:62
at Object.fire  https://es.m.wikipedia.org/w/load.php?lang=es&modules=mediawiki.base&skin=minerva&version=f42i5:4:699
at Object.<anonymous>  https://www.mediawiki.org/w/index.php?title=XTools/ArticleInfo.js&action=raw&ctype=text/javascript:39:37 👈
at fire  https://es.m.wikipedia.org/w/load.php?lang=es&modules=jquery%7Cmobile.startup&skin=minerva&version=bbkjp:42:705
at Object.fireWith [as resolveWith]  https://es.m.wikipedia.org/w/load.php?lang=es&modules=jquery%7Cmobile.startup&skin=minerva&version=bbkjp:43:903
at done  https://es.m.wikipedia.org/w/load.php?lang=es&modules=jquery%7Cmobile.startup&skin=minerva&version=bbkjp:127:417
at XMLHttpRequest.<anonymous>  https://es.m.wikipedia.org/w/load.php?lang=es&modules=jquery%7Cmobile.startup&skin=minerva&version=bbkjp:131:59

and indeed XTools-pageinfo.js does use the wikipage.content hook: mw.hook('wikipage.content').fire($result);,

I'm not sure why this is even executing on Minerva though, since that skin doesn't have a #contenSub element to show the pageinfo gadget

Looking at the revision history for XTools/ArticleInfo.js it looks a change to use this hook started at about the same time as this error spike, so I think this might be related to T358298 CC:@MusikAnimal

In T360781#9662645, @Jdrewniak wrote:

…
Looking at the revision history for XTools/ArticleInfo.js it looks a change to use this hook started at about the same time as this error spike, so I think this might be related to T358298 CC:@MusikAnimal

Is this a problem? Are we misusing the wikipage.content hook? It should only ever be returning a jQuery object for an HTMLElement.

I'm not sure why this is even executing on Minerva though, since that skin doesn't have a #contenSub element to show the pageinfo gadget

Thanks, you found a bug! I can make the adjustments for it to work in Minerva (as intended), but I will first wait to hear if anyone feels this is problematic. If necessary I can disable the gadget altogether in Minerva, which make the errors go away, but ideally the XTools gadget and Minerva could coexist peacefully.

@MusikAnimal I agree that it should only return a jQuery object for an HTMLElement. I added guards in Minerva to check for $content && $content[ 0 ] but it looks like that's not enough. Maybe it's because the lines
$result = $('#xtools_result'); returns undefined in Minerva because #contentSub doesn't exist, but that should still have been caught by $content[ 0 ]. I might add an additional check in Minerva for $content[0] instanceof HTMLElement .

Anyhow, regarding proper usage, the docs for this hook state the following:

/*
 *  @param {jQuery} $content The most appropriate element containing the content,
 *  such as #mw-content-text (regular content root) or #wikiPreview (live preview
 *  root)
 */

So I think it's expected that the hook returns the entire content section, not just the modified or additional content. articleInfo.js is a bit of an edge-case because it's not adding directly to #mw-content-text but rather to #contentSub, so returning $('#mw-content-text') would actually exclude the changes. I think instead, you could return both the content and the articleInfo HTML by adding the articleInfo HTML to the jQuery collection:

instead of:

mw.hook('wikipage.content').fire($result);

something like

$content = $('#mw-content-text').add( $result );
mw.hook('wikipage.content').fire($content);

Change #1015092 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/MinervaNeue@master] Follow-up to cf723c00 - Guard against undefined $container element

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

Change #1015092 abandoned by Jdrewniak:

[mediawiki/skins/MinervaNeue@master] Follow-up to cf723c00 - Guard against undefined $container element

Reason:

It looks like this hook is causes errors in more than just this one place.

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

@MusikAnimal just looking at the logs again, and it looks like this hook is causes errors in more than one place in Minerva and MobileFrontend. I don't see anything wrong with making it compatible with Minerva 👍
but, given the error rate, I think we should disable the gadget for this skin until that happens.

In T360781#9666628, @Jdrewniak wrote:

Anyhow, regarding proper usage, the docs for this hook state the following:

/*
 *  @param {jQuery} $content The most appropriate element containing the content,
 *  such as #mw-content-text (regular content root) or #wikiPreview (live preview
 *  root)
 */

So I think it's expected that the hook returns the entire content section, not just the modified or additional content.

A Codesearch and Global Search suggests the contrary (example), but indeed the docs are confusing. I think it makes sense for gadgets and such to expect what is passed in to be altered content, otherwise you unnecessarily (possibly) re-process the entire page every time the hook is fired.

articleInfo.js is a bit of an edge-case because it's not adding directly to #mw-content-text but rather to #contentSub, so returning $('#mw-content-text') would actually exclude the changes. …

It may still be an edge case in that regard, if $content given by the hook is expected to be within #mw-content-text, but I'm not sure if that's the case. Sounds like at minimum, we need some clarification in the docs!

just looking at the logs again, and it looks like this hook is causes errors in more than one place in Minerva and MobileFrontend. I don't see anything wrong with making it compatible with Minerva 👍
but, given the error rate, I think we should disable the gadget for this skin until that happens.

I've removed the hook entirely as a stopgap measure. Getting the script to work in Minerva is simple, but I'd prefer some reassurance usage of the wikipage.content hook in this way is acceptable or not. I nearly rejected T358298 because I was unsure, but the code searches convinced me otherwise.

hi @MusikAnimal I've tried to get some clarity on the expected usage of wikipage.content but I wasn't able to come to any definite conclusions. As you point out though, the defacto usage suggests that passing in just the changed html is OK.

Thanks for temporarily removing the hook as we get this sorted! 🙏

When running the ArticleInfo.js script on Minerva, (here through the firefox console) an error is easily reproducible:

However, this error also occurs just by passing an empty jQuery collection to the hook:

var $content = $( '#does-not-exist' );
mw.hook('wikipage.content').fire($content);

This means that this error is not entirely the fault of the XTools gadget, but rather the hook handling In Minerva/MobileFrontend. In these repos, we've strayed away from the safety of jQuery, opting to use native DOM functions instead. But, unlike their jQuey counterparts, these DOM functions throw an error on an empty jQuery collection. e.g. $('#does-not-exist').get( 0 ).addEventListener() throws whereas $('#does-not-exist').on( 'click' ) doesn't.

These hooks were actually introduced as a fix for T306705 and T219420. Given that any gadget/extension can call these hooks, it looks like these case should be fixed in Minerva/MobileFrontend regardless of whether or not the gadget calls this hook or not.

Change #1016421 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/MinervaNeue@master] Follow-up to cf723c00 - Guard against undefined $container

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

Change #1016428 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/extensions/MobileFrontend@master] Guard against undefined in lazyLoadedImages.js

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

Change #1016421 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Follow-up to cf723c00 - Guard against undefined $container

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

Sorry @Edtadros there's still one patch that needs to be merged, so I'm putting this back into code review.

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

Change #1016428 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Guard against undefined in lazyLoadedImages.js

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