1325907 - Flickr.com error: TypeError: e is undefined in combo:25:200

Status: RESOLVED INVALID

(Web Compatibility :: Site Reports, defect, P3)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

After load flickr.com there is a error in webconsole |TypeError: e is undefined in combo:25:200| and the website is nonfunctional.

I ran mozregression and the first known bad build is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8de88776f656609009d371e9b71ccea8c2fe7615&tochange=00fde28274fa2afb2bdaacc9152046269c7e9db8

waldo, it must be one of your changesets caused this.

Comment 1

[Jeff Walden [:Waldo]]

I think mozregression messed up somehow.  I get the error when I build and run 732548de4aad locally, and that's the very first revision in the "bad" range -- and it's not even my revision.  Maybe bisection on whatever range it is, needs to be done manually because of compiler-busted revs in there?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

I bisected again using git bisect and find Bug 1317422 - Implement a global 'global' property whose value is the global object.  r=jandem https://hg.mozilla.org/mozilla-central/rev/d37fc752b0af25f53e32d893739228a6cfc3b60b is the first bad commit. Reverting it fixed the website.

It looks like the YUI library used |global| as variable name in various places and this change breaks it because |global| cannot be overrided.

Comment 3

[Masatoshi Kimura [:emk]]

Yay, yet another "break the web" issue :(

That said, I don't understand why TC39 considered it is safe to add such a super-generic name to the global scope.

Comment 4

[ziyunfei]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #2)
> because |global| cannot be overrided.

|global| is a configurable and writable property, so it can be overridden.

This error is caused by:

> typeof global == "undefined" || typeof window != "undefined" && window !== global.window ? this : global

in https://s.yimg.com/zz/combo?uy/build/hermes-1.1.1359/moment/moment-min.js

and its source is here: https://github.com/moment/moment/blob/94ad539f965689289af54b6493abca0a65ab4ce6/moment.js#L15

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to ziyunfei from comment #4)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #2)
> > because |global| cannot be overrided.
> 
> |global| is a configurable and writable property, so it can be overridden.

Indeed you can override |global| in script file. I've only tested in web console and it always eval to the original global. It probably deserves another bug.

Comment 6

[Tom S]

Opened an issue on the proposal repo: https://github.com/tc39/proposal-global/issues/20. We should probably back this out for now.

Comment 7

[Jeff Walden [:Waldo]]

To summarize, then:

1. From the comments in the Github issue, it sounds like Yahoo is misusing moment.js.

2. They're embedding moment.js's source inside a function expression, like so:

YUI.add(
  "moment",
  function(e, t) { // module function
    (function(e) {

	var r = typeof global == "undefined" || (typeof window != "undefined" && window !== global.window)
	      ? this
	      : global;

        // ...
    }).call(this); // HERE

    // ...
  },
  "@VERSION@"
);

3. When the module function is called, it's not called with the global object as |this|.  Rather, it's some sort of YUI module object that looks like


 i = {
   name: e,
   fn: t,
   version: n,
   details: r
 },

with e/t/n/n corresponding to the three arguments passed to YUI.add.  (And |r| is an empty object, for this case.)

4. If |typeof global == "undefined"|, then |r| is the YUI module as desired, and things are okay.

5. But with us implementing |global|, it's not.  So now |!(typeof global == "undefined")| in the browser, and |!(typeof window != "undefined")| as well.

6. So moment is installed onto |global|.  That's the global object, not the YUI module object where moment *should* have been installed.

7. Eventually code wants to use the .moment, thinks it's on the module object but it's not, and we get a property deref on |undefined|.

Moving over to TE so they can poke Yahoo through channels about this.

Comment 8

[Mike Taylor [:miketaylr]]

Thanks for the explanation Jeff. Karl, can you get in touch with Yahoo! about this? Comment #7 explains the issue.

Comment 9

[Jeff Walden [:Waldo]]

As for a fix: I *think* changing

    .call(this)

to

    .call(Function("return this")())

should be an adequate fix, to make |this| be the global object as it would be if moment.js weren't embedded in a function that has the effect of changing what |this| would be.

I think this is portable across browsers, whether or not they support the global property, and on Node.

But 1) if you need compat outside of the browser environment, double-check me, and 2) this depends upon not using a content security policy https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP that would forbid runtime code generation.  If CSP does interfere, you should have some site-specific way to compute the global object that will reasonably substitute.

Comment 10

[Justin Dolske [:Dolske]]

This bug makes Flickr completely unusable for me on Nightly... What's the plan here? Are we expecting Flickr to fix their site, or is a backout an option?

Comment 11

[Mike Taylor [:miketaylr]]

Given that this also breaks deezer... this change seems compat-hostile. 

Tom, would you recommend opening a new bug for backing this out, or re-open the original landing bug?

Comment 12

[Tom S]

I suggest we back this out, I reopened the original bug.

Comment 13

[Mike Taylor [:miketaylr]]

Thanks (I still think we should leave this bug open and ask Flickr to make the suggested changes).

Comment 14

[Karl Dubost💡 :karlcow]

yes me too. We should ask for a change.
I'll do that this week. 
Do we know why the global breaks some sites?

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Karl Dubost :karlcow from comment #14)
> Do we know why the global breaks some sites?

Yes.  We've looked at them and diagnosed the reasons.

But in the sense I think you mean it, in the sense of one overarching answer: not really.  Each one of these sites is breaking for essentially a different ultimate reason.  The moment library has been implicated in this twice -- but once, because someone was embedding moment in a context that broke its assumptions, and a second time, because someone was using a two-year-old version of moment.  (It's also worth noting that two-year-old version is broken *even if this global property proposal isn't implemented*, if you have an element in your page with id="global" in it -- which motivated that two-year-old change.)  Bug 1326032 isn't the result of the moment library, but of different code that assumes presence-of-global means Node (and would also break if an element with id="global" were in the page).

Comment 16

[Karl Dubost💡 :karlcow]

Contacted someone at Yahoo! about it.

Comment 17

[Karl Dubost💡 :karlcow]

Thanks Jeff. That was the sense of my question.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This was fixed on the Gecko side by backing out bug 1317422.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Sorry, missed that we wanted to leave this open for the site to get fixed too.

Comment 20

[Karl Dubost💡 :karlcow]

fyi. Safari has shipped it in Safari Tech Preview 21

> Added support for the global property on the global object (r210052)
https://webkit.org/blog/7265/release-notes-for-safari-technology-preview-21/

Comment 21

[ziyunfei]

(In reply to Karl Dubost :karlcow from comment #20)
> fyi. Safari has shipped it in Safari Tech Preview 21
> 
> > Added support for the global property on the global object (r210052)
> https://webkit.org/blog/7265/release-notes-for-safari-technology-preview-21/

It has been reverted https://bugs.webkit.org/show_bug.cgi?id=166915

Comment 22

[Tom S]

We are now using "globalThis" instead of "global".