Page MenuHomePhabricator

Investigate impact and protection from errors in banners JS
Closed, ResolvedPublic

Description

It looks like catching errors from injected script tags may be possible. See:

https://stackoverflow.com/a/51857110/1165940

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror

Would this prevent an error in JS in a banner from halting JS execution on the whole pageview? What is the current situation? That is, how do errors in banner JS currently impact on the site, on mobile and desktop?

See T214330 for a recent issue of this sort.

Event Timeline

AndyRussG renamed this task from Investigate impact and protection from errors from scripts in banners to Investigate impact and protection from errors from in banners JS.Jan 22 2019, 4:16 PM
AndyRussG renamed this task from Investigate impact and protection from errors from in banners JS to Investigate impact and protection from errors in banners JS.

Would this prevent an error in JS in a banner from halting JS execution on the whole pageview?

No, script execution in web browsers is not capable of being halted (short of DevTools or other native interventions, not something a JS program can do form within the scope of a web page).

The worst that will happen is the current script (e.g. the banner itself) will abort its own execution, and the rest happily continues.

For example:

<script>alert(totallynot.here);</script>
<!--
 DevTools console and window.onerror receive:

 > Uncaught ReferenceError: Uncaught ReferenceError: totallynot is not defined

-->

<script>alert('yep');</script>
<!-- Second script executes without issue, alert message shown correctly. -->

<h1>Yep</h1>
<!-- Heading renders without issue -->

That is, how do errors in banner JS currently impact on the site, on mobile and desktop?

To a first approximation, they don't impact anything.

They will of course be measured by instrumentation tracking window.onerror, which is good but otherwise harmless.

In a nut shell:

  • JavaScript execution uses an event loop (great talk! *click* *click*).
  • Every tick of the event loop (mostly) has its own function call stack.
  • When an uncaught error happens, the originating stack is halted, discarded, and an error event is emitted. (and handlers such as window.onerror, are themselves queued to run in a future tick.)
  • Other ticks are unaffected.
  • Every "script" has its own tick (more or less).

@Krinkle , thanks much for the great explanation!

Heh indeed, thanks for sharing. :)

  • Every "script" has its own tick (more or less).

OK, this is good to know. So, just to put this in the MW context, an error in JS might affect other JS-based functionality only if it occurs in code executed in the same tick as initial processes, where it could halt modules loading or other essential setup (like the instantiation of API objects). Is that right?

I was seeing broken JS for certain pages, but it's possible of course these were unrelated to the banner...

I was seeing broken JS for certain pages, but it's possible of course these were unrelated to the banner...

We could add the banner, or any banner with a JS error, to a test campaign on aa.wikibooks (no longer active but is still live, can be used for CN tests), to test to be sure. @Krinkle, are there any per-platform caveats to what you described?

I've set up a test banner with a JS error on the beta cluster.

Banner content visible in the Beta-Cluster-Meta admin UI here.

It's set to show on all devices on Beta-Cluster-English-Wiktionary. Links:

desktop
mobile

You should see a banner that says, "This is a test banner with a script tag with a JS error," and some stuff in the console. You can also check that CN tried to load the banner by typing mw.centralNotice.getDataProperty( 'banner' ); in the console.

Please feel free to fiddle with the test banner content to see if other bad stuff in the banner could cause problems elsewhere...

Thanks!!!

As Timo predicted, I am not seeing any impact on any other JS on mobile or desktop.

As Timo predicted, I am not seeing any impact on any other JS on mobile or desktop.

Cool! Thanks for checking!!! :)

AndyRussG closed this task as Resolved.EditedSep 14 2019, 4:26 AM
AndyRussG claimed this task.

Closing, since it seems this issue is settled! Please re-open if I'm wrong... Thanks!!