1246827 - SolarEdge solar-panel monitoring dashboard does not finish loading, in Firefox builds that have "ES6 block-level functions" patches

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Log in to a SolarEdge account at https://monitoring.solaredge.com/  -- or, if you're already logged in, just visit https://monitoring.solaredge.com/

(I have an account for my home solar panels. I can send credentials privately to any trusted engineer working on this bug.)

EXPECTED RESULTS:
Site should load, showing "

ACTUAL RESULTS:
The site's background-image loads, but nothing else shows up the page. Additionally, this ends up in the Browser Console:
{
unreachable code after return statement     ext-base-debug.js:4806:4
unreachable code after return statement     ext-base-debug.js:4806:4
SyntaxError: redeclaration of let listener  main:1116:10
unreachable code after return statement     SE.Wrappers.js:222:9
unreachable code after return statement     main:1866:1
TypeError: sp is undefined                  ext-base-debug.js:264:21
}

Note: the "unreachable code" warnings are spammed in working builds as well, so I think they're unrelated.  The "redeclaration" and the "sp is undefined" errors seem to be the culprits here - they do not show up in working builds.

Script links:
=============
* ext-base-debug.js is "Ext JS Library 3.4.0 Copyright(c) 2006-2011 Sencha Inc."
https://monitoring.solaredge.com/solaredge-web/javascript25012016/ext-lib-3.4.0/ext-base-debug.js

* The "main" link is just script that's inline on the page itself (https://monitoring.solaredge.com/solaredge-web/p/main ) after I've logged in, but I suspect you can't look at that script without logging in.

Comment 1

[Shu-yu Guo [:shu]]

Most likely the redecl early error is screwing things up. Could you get me the main JS? I can't view it without logging in as you suspected.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> EXPECTED RESULTS:
> Site should load, showing "

Sorry, I forgot to finish this part of the description. :)  Meant to say: "Site should load, showing a header-bar & "dashboard" & "layout" buttons at the top, and more information progressively loading further down the page."


Also: I used mozregression to track down a regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dd319db81bb855825d851b344fd2da070f1a7e74&tochange=c7a3d4a1a2f817865caeb0004f918d77c728f91e

--> regression from bug 1071646.

I'll post the main JS as an attachment here.

Comment 3

[Daniel Holbert [:dholbert]]

The JS right around the "main" error is:

SE.ModuleLoader =
{
	hideAllPanels:function()
	{
		var panels=['dashboard','layout','alerts','charts','admin','layout','reports','control'];
		for(var i=0;i<panels.length;++i)
		{
			if(Ext.fly(panels[i])) Ext.fly(panels[i]).addClass('panel_no_display');
		}
	},
	loadDashboardPanel:function()
	{
		llog('@@ loadDashboardPanel @@');
		SE.util.display.setStdDisplay();
		
						 				var loadIFrame = false;
		if(loadIFrame == true)
		{
			var externalContextUrl = 'http://rndappsrv01:8080/solaredge-web';
												var sso = Ext.util.Cookies.get('SolarEdge_SSO-1.4');
			var iFrameURL = externalContextUrl + '/p/site/dashboard/218088?sso=' + sso;
			this.dashboardFrame = new Ext.ux.IFrame({id:'se-dashboard-frame',width:940,height:1,url:iFrameURL,renderTo:'dashboard'});
			Ext.fly('dashboard').removeClass('panel_no_display');
	    	function listener(event)
	    	{
	            switch(event.data.fn)
				{
				    case 'setHeight':
						this.dashboardFrame.setHeight(event.data.params.height);
				    break;
				}
	    	}
	    	var listener = listener.bind(this);

The error is spammed for the final line there.  Perhaps it's a name collision between "function listener" and "var listener"?  (Did bug 1071646 change something that would make that an error?)

Comment 4

[Daniel Holbert [:dholbert]]

(I won't post the 'main' script publicly after all, because it's got my personal info encoded in it. I'll email it to you, though, in case the answer is not in comment 3's snippet.)

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Daniel Holbert [:dholbert] from comment #3)

> The error is spammed for the final line there.  Perhaps it's a name
> collision between "function listener" and "var listener"?  (Did bug 1071646
> change something that would make that an error?)

Yeah, FiB (functions in block) means functions within blocks behave like 'let' bindings,

{ let x; var x; } is a redeclaration error, so 

{ function x() {} var x; } is a redeclaration error also.

We recently ran into compat problems and backed off on early error for redeclaring the same function twice, but did not back off of this behavior. Ugh.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (I won't post the 'main' script publicly after all, because it's got my
> personal info encoded in it. I'll email it to you, though, in case the
> answer is not in comment 3's snippet.)

No need, you did my debugging for me. :)

Comment 7

[Shu-yu Guo [:shu]]

So while we try backing off the early error in this case as well, this is straight up weird code and even with the early error backed off, it won't do what the developer intends.

I'm pretty sure the person who wrote |var listener = listener.bind(this)| intended to take the inner binding of "listener", call .bind(this) on it, and assign it to an outer var binding of "listener", which is straight up textually impossible to do in JS.

We should file a bug with SolarEdge.

Comment 8

[Daniel Holbert [:dholbert]]

Shu and I each reached out to @SolarEdgePV over Twitter, BTW:
 https://twitter.com/CodingExon/status/696864856306110464
 https://twitter.com/_shu/status/696863556524863488

Comment 9

[Daniel Holbert [:dholbert]]

(They also have a support system at http://www.solaredge.us/groups/service/support/?section=owner , but my account credentials didn't work there. Also, that login page isn't served securely :-/ )

Comment 10

[Mike Taylor [:miketaylr]]

I also sent a tweet (https://twitter.com/miketaylr/status/696915699378171904) to @philiparad (who according to his LinkedIn acocunt hacks on a ton of front-end JS and ExtJS stuff at SolarEdge). Hopefully he can help us find someone who can fix up the issues here.

Comment 11

[Daniel Holbert [:dholbert]]

Based on talking to Shu, it sounds like the easiest fix is for SolarEdge to tweak their JS in comment 3 to rename the function -- e.g. by adding an underscore at the end. So they could replace this...
>	    	function listener(event)
...with this:
>	    	function listener_(event)
          (note the underscore---^ )

And then, they'd just use that renamed function-name lower down in the bind expression, like so:
> 	    	var listener = listener_.bind(this);
         (again, note the underscore---^ )

Then there wouldn't be a naming collision.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Mike Taylor [:miketaylr] from comment #10)
> I also sent a tweet [...] to @philiparad

Good news -- Philip says, via email:
  I have followed your recommendation and fixed the bug.
  We will update production env. Next week.

Hooray! I'll add a reminder to my calendar to circle back in a little over a week to double-check that it's working in Nightly.

Comment 14

[Shu-yu Guo [:shu]]

Huzzah. Top sleuthing from Mike there.

Comment 15

[Daniel Holbert [:dholbert]]

(Retested today; this doesn't seem to be fixed yet, but I'll check again in another week and reach out to SolarEdge if it's still not fixed at that point.)

Comment 16

[Daniel Holbert [:dholbert]]

SolarEdge has fixed this on their end, applying the suggested change in comment 11. Hooray!