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

REOPENED
Unassigned

Status

Tech Evangelism
Desktop
P3
normal
REOPENED
a year ago
15 days ago

People

(Reporter: kanru, Unassigned)

Tracking

(Blocks: 1 bug, {regression, site-compat})

Trunk
regression, site-compat

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 unaffected)

Details

(Whiteboard: [sitewait])

(Reporter)

Description

a year ago
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.
Flags: needinfo?(jwalden+bmo)
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?
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 2

a year ago
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.
Blocks: 1317422
Flags: needinfo?(jwalden+bmo)
Keywords: site-compat
(Reporter)

Updated

a year ago
Version: 52 Branch → Trunk
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

a year ago
(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

Updated

a year ago
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
(Reporter)

Comment 5

a year ago
(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.
Opened an issue on the proposal repo: https://github.com/tc39/proposal-global/issues/20. We should probably back this out for now.
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.
Component: JavaScript Engine → Desktop
Flags: needinfo?(jwalden+bmo)
Product: Core → Tech Evangelism
Thanks for the explanation Jeff. Karl, can you get in touch with Yahoo! about this? Comment #7 explains the issue.
Flags: needinfo?(kdubost)
Whiteboard: [contactready]
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.
See Also: → bug 1326032
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?
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?
Flags: needinfo?(evilpies)
I suggest we back this out, I reopened the original bug.
Flags: needinfo?(evilpies)
Thanks (I still think we should leave this bug open and ask Flickr to make the suggested changes).
yes me too. We should ask for a change.
I'll do that this week. 
Do we know why the global breaks some sites?
(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).
Contacted someone at Yahoo! about it.
Thanks Jeff. That was the sense of my question.
Flags: needinfo?(kdubost)
This was fixed on the Gecko side by backing out bug 1317422.
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Sorry, missed that we wanted to leave this open for the site to get fixed too.
Status: RESOLVED → REOPENED
status-firefox53: fixed → unaffected
Resolution: FIXED → ---
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

a year ago
(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
Whiteboard: [contactready] → [sitewait]
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.