1448046 - Can we remove the window.Components shim?

Status: REOPENED

(Core :: DOM: Core & HTML, task, P3)

Description

[Boris Zbarsky [:bzbarsky]]

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-03-01&keys=__none__!__none__!__none__&max_channel_version=beta%252F59&measure=USE_COUNTER2_DEPRECATED_Components_PAGE&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2018-01-22&table=0&trim=1&use_submission_date=0 says it's used on .06% of pageloads on beta 59.

Another useful metric is https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-03-01&keys=__none__!__none__!__none__&max_channel_version=beta%252F59&measure=COMPONENTS_SHIM_ACCESSED_BY_CONTENT&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2018-01-22&table=0&trim=1&use_submission_date=0 which says that .66% of browsing sessions encountered it at least once (still on beta 59).

Unlike the controllers shim, this one is present on nightly too.  I'm going to try disabling it on nightly and see whether we run into any breakage...

Comment 1

[Mike Taylor [:miketaylr]]

I ran into a usage of Components (doing research for something else) as a proxy or UA sniffing, in http://fukutsu-aeonmall.com/js/am_aeonmall.js

O.L3=function(n){switch(n){case 'ie':p='execScript';break;case 'ff':p='Components';break;case 'op':p='opera';break;case 'sf':case 'gc':case 'wk':p='defaultstatus';break;}return p&&window[p]!==undefined;}

There's 186 instances of "aeonmall.js" in https://docs.google.com/spreadsheets/d/1zxE-jEaYsyCkYTi99HuatJ4WEnAcD2GOrudXEIp9qbs/edit#gid=1464363467

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, but are any of those pages broken on nightly (where we don't ship a Components)?

Comment 3

[Mike Taylor [:miketaylr]]

Yeah, I don't know that yet. :) Just documenting the existence before I forget.

Comment 4

[Thorin [:thorin]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #0)

.06% of pageloads on beta 59
.66% of browsing sessions encountered it at least once (still on beta 59)

FYI: some updated % figures for 71

• USE_COUNTER2_DEPRECATED_Components_PAGE - Dev 71 was 0.87, Beta 71 was 0.31 (that's Boris' .06 above)
• USE_COUNTER2_DEPRECATED_Components_DOCUMENTS - Dev 71 was 0.40, Beta 71 was 0.08
• COMPONENTS_SHIM_ACCESSED_BY_CONTENT - Dev 71 was 2.76, Beta 71 was 2.06 (that's Boris' .66 above)

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's really depressing. :(

Comment 6

[Thorin [:thorin]]

Updating the depressing news: all Beta

• USE_COUNTER2_DEPRECATED_Components_PAGE - 88 = 0.30, 89 = 0.34
• USE_COUNTER2_DEPRECATED_Components_DOCUMENTS - 88 = 0.07, 89 = 0.08
• COMPONENTS_SHIM_ACCESSED_BY_CONTENT - 88 = 0.75, 89 = 0.79

At least the last measurement has come back somewhat to FF59 levels, hooray!

Comment 7

[Tom Ritter [:tjr]]

I'm not sure what this is exactly; but it's been disabled on Nightly for a few years now, and seems like some tech debt we wanted to get rid of that that fell off the radar. Maybe levels are low enough. Brian, do you have more context on this?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #7)

I'm not sure what this is exactly; but it's been disabled on Nightly for a few years now, and seems like some tech debt we wanted to get rid of that that fell off the radar. Maybe levels are low enough. Brian, do you have more context on this?

Sorry for the delay. I don't have any insight but it's a good question since we've been disabled so long in nightly. Karl, do you have any thoughts here?

Comment 9

[Karl Dubost💡 :karlcow]

so for the thing that mike found in the past about the AEON store in Japanese. This is still used.

  O.L3 = function (n) {
    switch (n) {
      case 'ie':
        p = 'execScript';
        break;
      case 'ff':
        p = 'Components';
        break;
      case 'op':
        p = 'opera';
        break;
      case 'sf':
      case 'gc':
      case 'wk':
        p = 'defaultstatus';
        break;
    }
    return p && window[p] !== undefined;
  };

To the question does it break the Aeon site, I don't know. I don't see anything obvious that would break.
BUT if it's only the Aeon site, then we could easily shim it with a site intervention only for the aeon site.

And from a quick check we didn't have any bugs caused by this on webcompat.

Maybe to double check with addons team. I don't know if Mozilla Addons team has the possibility to quickly parse code from addons on the store to double check if window.Components is being used.

So I would say yes. Let's remove it.

Comment 10

[Thorin [:thorin]]

So I would say yes. Let's remove it.

How about rolling it into Beta/Dev and see what happens?

Comment 11

[Tom Ritter [:tjr]]

Attachment: Bug 1448046: Disable the components shim on all channels r?bgrins

Comment 12

[Dennis Schubert [:denschub]]

Leaving a comment in this bug instead of the patch so that it is more visible.

We have use counters, both on a per-page-load basis, and a per-session basis. For the entire 112 Beta cycle, it looks like 0.259% of page loads hit the shim, and 1.06% of all sessions respectively. This would be a significant increase from the numbers bz shared in comment 0. I can't really say that there is a trend in any direction, it's a bit noisy.

That being said, there could be a lot of reasons why this is what we're seeing. It could be fingerprinting, in which case it wouldn't be a concern. It could be a legacy browser detection, which would be a concern. Hard to say without spending more time digging into this. Going forward, we have two options:

1. Disable the shim in Beta for a while and see if any reports pop up.
2. Just disable it in all channels and hope nothing breaks.

Given this has been disabled in Nightly for quite some time, I'd personally be fine with option 2. With this being a pref, we always have the "worst-case" resolution of toggling the pref back with a remote pref flip. Site breakage like the one mentioned in comment 9 are a concern, but unless it's a huge fire, we can ship interventions to the known breakage. If it's too much, we would have to find a different solution, but I wouldn't block this based on this instance.

Comment 13

[Dennis Schubert [:denschub]]

To add to my previous message: it's possible that the telemetry numbers are completely inflated. I ran a code search on GitHub, and noticed that storing custom UI component logic on window.Components is not a rare thing developers do. I tested, and JS like

window.Components = { hello: "world" };
alert(window.Components.hello);

will increase the use counters. Not all code snippets I found use the uppercase variant, but some definitely do.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3101658ddd48
Disable the components shim on all channels r=webcompat-reviewers,denschub

Comment 15

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/3101658ddd48

Comment 16

[Andrew McCreight [:mccr8]]

I think this should have gotten DOM peer review before landing.

Comment 17

[Peter Van der Beken [:peterv]]

Yeah, a DOM peer review probably should have happened.

(In reply to Dennis Schubert [:denschub] from comment #12)

That being said, there could be a lot of reasons why this is what we're seeing. It could be fingerprinting, in which case it wouldn't be a concern. It could be a legacy browser detection, which would be a concern. Hard to say without spending more time digging into this.

So the numbers went up, but we don't really know why they're still a bit high? (We could probably detect and count code like comment 13 if we want to be sure)

1. Disable the shim in Beta for a while and see if any reports pop up.
2. Just disable it in all channels and hope nothing breaks.

What's the downside to being prudent, and doing 1 first? Especially given that those numbers went up?

Comment 18

[Dennis Schubert [:denschub]]

So the numbers went up, but we don't really know why they're still a bit high?

Yeah, exactly. Given this has been turned off in Nightly for a long time and we haven't seen any reports besides the stuff mentioned here, I'm confident that this will not break the entire internet, but we don't know for sure.

What's the downside to being prudent, and doing 1 first? Especially given that those numbers went up?

From WebCompat's perspective there is none. If we want to get rid of the shim, then we'd probably need to come up with a timeline for removal in Release as well, or otherwise it'll be just another pref that sticks around for a long time.

I'll note, however, that while our Beta user-base is larger than the Nightly user-base, it's still relatively small (and still very biased). It's a good canary to test if the shim removal could turn into a big fire, but I wouldn't be surprised if there is a more widespread issue, we only see a larger number of reports if this is in the Release channel. I'm absolutely not opposed of being careful and shipping this to Beta for a couple of cycles, though. :)

Comment 19

[Brian Grinstead [:bgrins]]

Given Comment 17 and 18 perhaps we should land a new patch to disable the shim on beta only (we're still in 115 - no uplift would be needed), and then handle disablement in all channels in a separate bug after shipping on beta (seems like 1 version is probably enough to collect any major issues but the specifics can be discussed more).

Peter, what do you think? Tom, do you have any sense of urgency for this change that hasn't been communicated here? Seems like this is mostly a tech debt cleanup that could wait for a cycle or two, as long as we are diligent to reassess and not leave it in limbo.

Comment 20

[Natalia Csoregi [:nataliaCs]]

Backed out as requested: https://hg.mozilla.org/mozilla-central/rev/fc304dfe9ba9a6dfe11923111a6b6edf0b9c2526

Comment 21

[Tom Ritter [:tjr]]

No, there is no urgency here - I've had the patch backed out. There is no macro for just beta though, you can only do Early Beta and Nightly.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The needinfo was requested 10 months ago. The usage data could change a lot. Clearing the needinfo for now.
If there's urgency of looking into this again, feel free to ask for a new request. Thanks.