Closed Bug 1561036 Opened 5 years ago Closed 5 years ago

xmlDoc.onload doesn't get fired causing a difference in site appearance between Chrome and Firefox Nightly

Categories

(Core :: DOM: Core & HTML, defect)

68 Branch
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1563839
Webcompat Priority ?
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- fixed
firefox67 --- unaffected
firefox68 + fixed
firefox69 + fixed
firefox70 + fixed

People

(Reporter: ksenia, Unassigned)

References

(Regression, )

Details

(Keywords: regression, webcompat:site-wait)

As described here:

https://github.com/webcompat/web-bugs/issues/33462

Steps to reproduce:

Go to https://rolb.santanderbank.com/FORPAS_ENS/ChannelDriver.ssobto?dse_operationName=forgotPwd&dse_parentContextName=&dse_processorState=initial&dse_nextEventName=start and observe the form

Expected:
Form labels appear

Actual:
Form labels are empty

Regression window:

7:47.43 INFO: Last good revision: 2b2554ff8f8c3d87e5035316e67f3a213fd11c54
7:47.43 INFO: First bad revision: f5273f8e51966ff5d45588bfcd1826a5642ba8b4
7:47.43 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2b2554ff8f8c3d87e5035316e67f3a213fd11c54&tochange=f5273f8e51966ff5d45588bfcd1826a5642ba8b4

Affected code xmlDoc.onload = this.onLoad; in https://rolb.santanderbank.com/Estatico/Globales/V74/Scripts/jsDomM.bjs

Flags: needinfo?(ehsan)

In the script mentioned in comment 0, the site has the following broken UA sniffing code:

getTypeBrowser: function (){
  /* Comp.Nav: Compatibilidad de navegadores */
  if(navigator.appName=='Netscape' && navigator.userAgent.indexOf('WebTV') == -1){
    if(parseInt(navigator.appVersion)==3 && (navigator.userAgent.indexOf('Opera') == -1 )){
      	this.brw = 'NN3';
      	this.subBrw = 'unknown';
      	this.cssEnabled = false;
    } else if( (parseInt(navigator.appVersion) >= 4)&& (navigator.userAgent.indexOf('Safari') == -1 )){
      	this.brw = parseInt(navigator.appVersion) == 4 ? 'NN4' : 'NN5';
      	if( navigator.userAgent.indexOf('like Gecko') != -1 ){
            this.subBrw = 'standard';
        } else {
            this.subBrw = 'unknown';
        }
    } else if (navigator.userAgent.indexOf('Safari/4') != -1  ) {
      	this.subBrw = 'S2';
      	this.brw = 'NN';
    } else if (navigator.userAgent.indexOf('Safari/') != -1  ) {
        this.subBrw = 'S3';
        this.brw = 'NN';
    }
  } else if(navigator.appName == 'Microsoft Internet Explorer'){
	    this.brw = (parseInt(navigator.appVersion) >= 4)?'IE5':'IE4';
    	this.subBrw = 'unknown';
  } else if(navigator.appName == 'Opera' && parseInt(navigator.appVersion) >= 5) {
	  	this.brw = 'O5';
    	this.subBrw = 'unknown';
  } if( this.brw.indexOf('NN')!=-1 ) {
 		this.brw = 'NN';
  } 
  return this.brw;
			
}			

Funny things to note about this code:

  • The missing else keyword before the last if keyword causes NN3, NN4 and NN5 to be replaced with NN!
  • This code predates both Firefox and Chrome, it seems.

Anyways, when we run this mess of a UA sniffing code, we get this.brw == "NN"; this.subBrw == "unknwon";. When Chrome runs it, they get this.brw == "NN"; this.subBrw == "S3" (IOW, the code thinks Chrome is Safari 3 and we're some futuristic unknown Netscape based browser.)

Then we get to run this function:

importXML: function (fileXML,docRoot){
 	try{
		var xmlDoc = null;
		this.objXMLHttpRequest = null;
		var withoutActiveX = false;
		
		if (window.XMLHttpRequest && !window.ActiveXObject)
       	{
       		
       		if(this.brw == 'O5' || this.subBrw == 'S2' || this.subBrw == 'S3'|| this.subBrw == 'standard'){
       			
				this.objXMLHttpRequest = new XMLHttpRequest();
				if (this.objXMLHttpRequest.overrideMimeType)
					this.objXMLHttpRequest.overrideMimeType('application/xml');
				var jsDom = this;
				this.objXMLHttpRequest.onreadystatechange=function()
				{
					jsDom.loadAsyncResponse(jsDom,fileXML);
				};
				
				this.objXMLHttpRequest.open('GET',fileXML,false); 
				this.objXMLHttpRequest.send(null);
			}	
			else if (document.implementation && document.implementation.createDocument)
			{
				
				xmlDoc = this.getOut().implementation.createDocument('', 'doc', null);
				xmlDoc.async=false;
				xmlDoc.onload = this.onLoad;
			}	
		}
		else {
			if (window.ActiveXObject) {
				var sig = ["MSXML2.DOMDocument.5.0", "MSXML2.DOMDocument.4.0",
						  "MSXML2.DOMDocument.3.0", "MSXML2.DOMDocument",
						 "Microsoft.XmlDom"];
				for (var i=0; i < sig.length; i++) {
					try {
						xmlDoc = new ActiveXObject(sig[i]);
						break;
					 } catch (oE){}
				}
				if (xmlDoc==null) {	withoutActiveX =true; }
				if( !withoutActiveX && xmlDoc.async){
					xmlDoc.async=false;
					xmlDoc.setProperty('ForcedResync',false);
					xmlDoc.validateOnParse=false;
					xmlDoc.onreadystatechange = this.onLoad;
				} 
			}
			else withoutActiveX=true;

			if (withoutActiveX){
				var onLoadWithoutActiveX = function() {
					if (typeof document.all[fileXML] != 'undefined' && 
						document.all[fileXML].readyState == 'complete' ) {
						xmlDoc = document.all[fileXML].XMLDocument;
					}
				}
				var body = document.createElement("body");
				document.appendChild(body);
				var xml = document.createElement("xml");
				xml.onreadystatechange = onLoadWithoutActiveX;
				xml.id = fileXML;
				xml.src =  fileXML;
				xml.async = false;
				document.body.appendChild(xml);
				document.body.removeChild(xml);
				document.removeChild(body);
				this.xmlDocs[fileXML] = xmlDoc;
				this.onLoad();
				dom.setLangLiteral(xmlDoc);
			}
		}
		if ((this.objXMLHttpRequest==null && xmlDoc!=null) || (!withoutActiveX && xmlDoc != null)){
			this.xmlDocs[fileXML] = xmlDoc;
		    (typeof xmlDoc.async!='undefined')?xmlDoc.load(fileXML):xmlDoc.load(fileXML,this.getOut(),docRoot);
			dom.setLangLiteral(xmlDoc);
	    }
	}catch(e){
		dump(e.message+" importXML",0);
	}		
			
},			

So we enter the buggy code path which relies on XMLDocument.onload as well as XMLDocument.async because of this.subBrw == "unknown", aka the "futuristic unknown version" part of being a Netscape based browser.

Has Regression Range: --- → yes
Has STR: --- → yes
Regressed by: 1328138

That site is very likely also broken in modern IE.

So in terms of fixing this bug, we have three different options that I can think of:

  1. Reaching out to the web site asking them to fix their UA sniffing code. They can do that easily by changing this condition navigator.userAgent.indexOf('like Gecko') != -1 in getTypeBrowser to look like navigator.userAgent.indexOf('like Gecko') != -1 || navigator.userAgent.indexOf('Firefox/') != -1. From the looks of the code it seems quite old, so I'm not certain how reasonable it would be to expect the site to fix its bug here.
  2. Adding a site-specific UA override. For example, setting the general.useragent.override.rolb.santanderbank.com pref to Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) like Gecko/20100101 Firefox/69.0 (which just injects like behind Gecko in our default UA string) fixes this bug by putting us in the same path in their UA sniffing code as the fix I suggested above.
  3. Set the dom.xmldocument.async.enabled and dom.xmldocument.load.enabled prefs back to true on 68 and maybe 69, pending perhaps a response from the website. That's the least desirable option since it defeats our intention to remove these Firefox-only APIs.

Needinfoing some webcompat experts to solicit opinions on how we should approach this... Thanks, and sorry about the needinfo spam.

Flags: needinfo?(wisniewskit)
Flags: needinfo?(miket)
Flags: needinfo?(kdubost)

(In reply to ExE Boss from comment #2)

That site is very likely also broken in modern IE.

Interestingly the labels all appear with the latest IE, Edge and Edgium on my machine...

Based on our desired outcome, to me it sounds like we should do #1 and #2. That is, we should add a site patch while we try to get the site to fix their broken sniff, just in case they're still able and willing.

Flags: needinfo?(wisniewskit)

(In reply to Thomas Wisniewski [:twisniewski] from comment #5)

Based on our desired outcome, to me it sounds like we should do #1 and #2. That is, we should add a site patch while we try to get the site to fix their broken sniff, just in case they're still able and willing.

Thanks!

Do we have any guidance about how to write a patch to do #2 given that the modifications to the UA string will be platform and version dependent?

For #1, do I need to file a separate bug? If yes, what would be the right component?

We'll likely follow the same kind of procedure as we did for bug 1464106, filing a bug under Web Compatibility/Interventions, making a pull request against the addon's GH repository, and then merging into m-c from there.

But could you provide the specifics on what combinations of platform/etc should produce a certain UA string? Based on that we can figure out what alterations to the appropriate file are necessary, and how we'd like to proceed (in case we want to refactor the code a bit more instead of just adding a new chunk to that config).

Flags: needinfo?(kdubost)

I'll do #1 and try to reach out to the site.

Depends on: 1563839

(In reply to Thomas Wisniewski [:twisniewski] from comment #7)

We'll likely follow the same kind of procedure as we did for bug 1464106, filing a bug under Web Compatibility/Interventions, making a pull request against the addon's GH repository, and then merging into m-c from there.

But could you provide the specifics on what combinations of platform/etc should produce a certain UA string? Based on that we can figure out what alterations to the appropriate file are necessary, and how we'd like to proceed (in case we want to refactor the code a bit more instead of just adding a new chunk to that config).

Thanks, I filed bug 1563839 with what I believe should be the necessary information. If there is anything missing please needinfo me there?

(In reply to :Ehsan Akhgari from comment #9)

(In reply to Thomas Wisniewski [:twisniewski] from comment #7)

We'll likely follow the same kind of procedure as we did for bug 1464106, filing a bug under Web Compatibility/Interventions, making a pull request against the addon's GH repository, and then merging into m-c from there.

But could you provide the specifics on what combinations of platform/etc should produce a certain UA string? Based on that we can figure out what alterations to the appropriate file are necessary, and how we'd like to proceed (in case we want to refactor the code a bit more instead of just adding a new chunk to that config).

Thanks, I filed bug 1563839 with what I believe should be the necessary information. If there is anything missing please needinfo me there?

SGTM

Flags: needinfo?(miket)

(In reply to Ksenia Berezina [:ksenia] from comment #8)

I'll do #1 and try to reach out to the site.

Ksenia sent me some LinkedIn contacts and I've messaged 3 individuals who work at Santander Bank N.A.

Flags: needinfo?(ehsan)

Now that bug 1563839 is on autoland, what is the process for fixing the regression on beta and release? Normal code uplifts?

(In reply to :Ehsan Akhgari from comment #12)

Now that bug 1563839 is on autoland, what is the process for fixing the regression on beta and release? Normal code uplifts?

For beta, a simple uplift will do. For release, we either need to get PI to test the XPI and get relman to sign off for a balrog deploy, or we do a dot release ridealong. I'll ping relman and ask if there are upcoming dot releases planned -- that might be faster if something is in the pipeline.

Thanks!

Since there is nothing more to be done here, I'm gonna resolve this bug as a duplicate of bug 1563839.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE

I called the bank today and tried to get their IT department to look at this issue. Not sure how successful I was but they have all the right information now.

:adamopenweb
Hi. My name is Jose Maria de Leon and I work for Portals in Santander UK IT division .

I have received an email from Santander US about this issue sent by you. First of all, thanks for informing us. Can you please clarify for me please? Has this issue being resolved in all releases but v68.0.0? Do we need to involve our IT Team? I'm asking the OLB team and they are not aware of this issue although I understand it will be raised soon. And from a Public site perspective www.santander.co.uk we haven't received any incident yet

Thanks

Flags: needinfo?(astevenson)

Hi again. I've just been informed about the issue in OLB for Corporate. Our CSA maintenance team is dealing with it. I understand that if Mozilla is fixing this for the newer versions this will not then conflict with our fixes...

Regards

Chema, we are only temporarily working around the issue so your site continues to work, while your IT team has time to make a proper fix.

Basically, Firefox no longer supports two web features that are non-standard, as can be seen here: https://www.fxsitecompat.dev/en-CA/docs/2019/xmldocument-load-and-xmldocument-async-have-been-removed/

Your site is relying on at least one of those features when it detects Firefox, and it no longer has to do that (our temporary work-around just bypasses that check for Firefox on your site, causing it to fall back to using standard web features instead).

Flags: needinfo?(astevenson)

Chema, thanks for getting back to us. Please note that in order to fix this issue, you can find the necessary information in comment 1. The problem we encountered is the old User-Agent sniffing code inside the function getTypeBrowser in https://rolb.santanderbank.com/Estatico/Globales/V74/Scripts/jsDomM.bjs which doesn't support parsing the Firefox User-Agent string (documented here). In order to work around the problem temporarily, we changed the User-Agent string that we send to your website to include like Gecko instead of Gecko in order to make it work with the current code in jsDomM.js.

Fixing this on your end requires updating the getTypeBrowser function to enable it to parse the Firefox User-Agent string correctly. One way to do that could be by looking for the "Gecko/" string inside User-Agent. Once this code has been updated on your end, we can remove the temporary work-around.

For testing purposes, you can see and control this temporary work-around in Firefox by typing about:compat in the browser address bar. Inside there you should see a corresponding entry for "rolb.santanderbank.com".

Hope this helps.

Thanks a lot Thoms and Ehsan

I have passed all this info to the labs in Madrid to resolve this for all versions of the CSS we use. One more question for our peace of mind. Has the release with the temporary fix already been released or when do you expect to release it?

Regards
Chema de Leon

Chema, it appears that the temporary fix is due to land with the next dot-release of Firefox (68.0.2), based on what I'm seeing in bug 1567198. It likewise should end up in the next beta release, and is already in the nightly builds.

Thanks Thomas

Do you more or less know when this beta will be available to the public. I don't really know what do you mean by nightly bilds :). We are already applying our fixes in all applications affected but our internal process is unfortunately kind of slow...

Sorry for the confusion! Basically, each release of Firefox starts off as nightly builds, then goes to beta, and then finally to release users.

We're hoping to get the fix our over the next week or two as part of Firefox 68.0.2, and I'll keep you updated.

However, I've just been told that we're not 100% sure if our temporary fix is covering all of the domains we need to cover. Could you confirm if these domains are all of the ones we might have to worry about?

If you or your engineers could verify this list, that could help speed things up (hopefully we are not missing anything!)

Thanks again for your quick response from the far wild west :)

Yes, I can confirm that those domains are correct

I will inform my UK teams to speed up the update of our software.

You have been very helpful.
Regards

That's good to hear!

In that case, I have even better news: the fix is already being sent out to all Firefox users, even before the next dot-release. (But some users may not get it until the next dot-release).

Hiagain Thomas

With this news I yesterday asked our LCO to test again but unfortunately the situation was the same as before for everyone. So, its either we are part of those some users that will get the change with the dot releasae or the change hasn't really being released. I've alseo just tried this morning at home to and same situation.

You need to log in before you can comment on or make changes to this bug.