Closed Bug 1325907 Opened 4 years ago Closed 2 years ago error: TypeError: e is undefined in combo:25:200


(Web Compatibility :: Desktop, defect, P3)



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

Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- unaffected


(Reporter: kanru, Unassigned)



(Keywords: regression, site-compat, Whiteboard: [sitewait])

After load 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

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)
I bisected again using git bisect and find Bug 1317422 - Implement a global 'global' property whose value is the global object.  r=jandem 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: globalThis
Flags: needinfo?(jwalden+bmo)
Keywords: site-compat
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.
(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


and its source is here:
(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: 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:

  function(e, t) { // module function
    (function(e) {

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

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

    // ...

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(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 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: → 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.
Closed: 4 years ago
Resolution: --- → FIXED
Sorry, missed that we wanted to leave this open for the site to get fixed too.
Resolution: FIXED → ---
fyi. Safari has shipped it in Safari Tech Preview 21

> Added support for the global property on the global object (r210052)
(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)

It has been reverted
Whiteboard: [contactready] → [sitewait]
Priority: -- → P3
We are now using "globalThis" instead of "global".
Closed: 4 years ago2 years ago
Resolution: --- → INVALID
Product: Tech Evangelism → Web Compatibility
You need to log in before you can comment on or make changes to this bug.