1897150 - Tables are not sorted in promete.fr after bug 1884360

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[info]

Attachment: v125 versus v126.png

Steps to reproduce:

upgrade from version 125 to 126

Actual results:

design on my website is not correctly displayed anymore ; datatable (http://datatables.net) component is partially broken : column cannot be draft, icons disappeared, data ordering is broken...

I suspect, this is linked with zoom property implementation but cannot certify it.

Expected results:

datatable component should behave like on v125 or on other browsers

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Can you confirm that, on 126, going to about:config and changing:

• layout.css.zoom.enabled to false
• layout.css.prefixes.transforms to true
• layout.css.prefixes.transitions to true

Fixes the issue? If so, then it'd confirm at least being related to it.

Having access to a test-case would be ideal... Is there any non-production environment I could get access too or something? If not, is there any chance you could construct a standalone page that reproduces the issue?

Thanks.

Comment 2

[info]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

I changed those 3 settings and it doesn't improve situation

• layout.css.zoom.enabled to false
• layout.css.prefixes.transforms to true
• layout.css.prefixes.transitions to true

I can still work on generating an non-production environment account for you to test ; but I am maybe wrong about the problem

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Cool, then it seems it's not relevant to zoom itself. Now the problem is we don't know what caused this.

A repro would still be the most useful path forward. Another alternative to figure out what change caused it would be using mozregression, which should take you ~15 minutes or so on a good internet connection.

That should pinpoint which commit caused this, but diagnosing the bug might not be obvious still, and a repro might be needed. Plus if you provide a repro, I can run mozregression for you :)

Comment 4

[info]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I sent you credentials by email as I don't know who / how many persons can read this ticket ;)

I found another problem, still on the same component (an hidden colum that is not hidden anymore) ;)

I think the js script has some problem(s) with v126 :

Uncaught SyntaxError: private names aren't valid in this context meteo.inte.promete.fr:1333:12
DataTables warning (table id = 'example'): Unexpected number of TD elements. Expected 459 and got 837. DataTables does not support rowspan / colspan in the table body, and there must be one cell for each row/column combination. jquery.dataTables.js:5726:13
Uncaught TypeError: this.ClosureDataTables is undefined
    jQuery 7
        fn
        _fnSort
        _fnInitalise
        dataTable
        each
        each
        dataTable
    <anonymous> https://meteo.inte.promete.fr/:229
    jQuery 4
        ready
        ready
        each
        ready
jquery.dataTables.js:1:60
    jQuery 7
        fn
        _fnSort
        _fnInitalise
        dataTable
        each
        each
        dataTable
    <anonyme> https://meteo.inte.promete.fr/:229
    jQuery 4
        ready
        ready
        each
        ready

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Can you double check? I don't think I got it.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Alternatively feel free to drop it here checking the "private" checkbox, which will make it accessible to only a few Mozilla employees with security access

Comment 7

[info]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Alternatively feel free to drop it here checking the "private" checkbox, which will make it accessible to only a few Mozilla employees with security access

My fault, email was still in the draft bin. I can't see the private checkbox, I sent the email right now. Sorry again.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

[Tracking Requested - why for this release]: Regression in JS built-ins, somehow, which could affect multiple websites.

Mozregression says: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=24e3adeb06bfe8c0f5c6e311f01b265fecdbff07&tochange=0ebc91ec13c2d8b82f7af1789bf616642dfc9502

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Console says:

Uncaught TypeError: can't access property "sort", this.ClosureDataTables is undefined
    jQuery 7
    <anonymous> https://meteo.inte.promete.fr/agroclim/display_data_station:834
    jQuery 4

So maybe something about how the prototype is set-up changed? Jan, mind taking a look?

Comment 11

[Emilio Cobos Álvarez [:emilio]]

FWIW on a debug build I get a tiny bit more info on stderr:

JavaScript error: https://meteo.inte.promete.fr/sites/all/modules/promete_utils/dataTables/media/js/jquery.dataTables.js?1715880065 line 4290 > eval, line 1: TypeError: can't access property "sort", this.ClosureDataTables is undefined

Maybe something to do with eval?

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1884360

Comment 13

[Iain Ireland [:iain]]

It looks like this is the code that's going wrong:

this.ClosureDataTables = {
	"fn": function(){},
	"data": aoData,
	"sort": _oExt.oSort,
	"master": oSettings.aiDisplayMaster.slice()
};
var sDynamicSort = "this.ClosureDataTables.fn = function(a,b){"+
	"var iTest, oSort=this.ClosureDataTables.sort, "+
	"aoData=this.ClosureDataTables.data, "+
	"aiOrig=this.ClosureDataTables.master;";

for ( i=0 ; i<aaSort.length-1 ; i++ )
{
	iDataSort = oSettings.aoColumns[ aaSort[i][0] ].iDataSort;
	iDataType = oSettings.aoColumns[ iDataSort ].sType;
	sDynamicSort += "iTest = oSort['"+iDataType+"-"+aaSort[i][1]+"']"+
		"( aoData[a]._aData["+iDataSort+"], aoData[b]._aData["+iDataSort+"] ); if ( iTest === 0 )";
}

if ( aaSort.length > 0 )
{
	iDataSort = oSettings.aoColumns[ aaSort[aaSort.length-1][0] ].iDataSort;
	iDataType = oSettings.aoColumns[ iDataSort ].sType;
	sDynamicSort += "iTest = oSort['"+iDataType+"-"+aaSort[aaSort.length-1][1]+"']"+
		"( aoData[a]._aData["+iDataSort+"], aoData[b]._aData["+iDataSort+"] );"+
		"if (iTest===0) "+
			"return oSort['numeric-asc'](jQuery.inArray(a,aiOrig), jQuery.inArray(b,aiOrig)); "+
		"return iTest;}";
	
	/* The eval has to be done to a variable for IE */
	eval( sDynamicSort );
	oSettings.aiDisplayMaster.sort( this.ClosureDataTables.fn );
}

Which is weird, because it looks like we're somehow going wrong in code that boils down to:

this.closureDataTables = { fn: function() {} };
var sDynamicSort = "this.closureDataTables.fn = ..."
eval(sDynamicSort);

...because inside the eval we don't see a closureDataTables property on this.

Edit: Poking at it in the debugger, it seems like this is the number 1 here somehow? That seems like a problem.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Reduced test-case.

This reproduces the issue in the devtools console. Hope it helps? :)

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Reduced test-case with the last two characters that I missed above.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Attachment: JS shell test-case.

This reproduces on the JS shell.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Attachment: More reduced test-case.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Attachment: As reduced as it gets.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Iain said he knew where the bug lives so I'll stop debugging now. I only got so far as to realizing that we were overriding the this value here :)

Comment 20

[Iain Ireland [:iain]]

This is a bug in the new trampoline-based implementation of array sort.

When calling a comparator function during an array sort, this should always be undefined (step 4 here), which will be replaced with the global in non-strict mode (step 6 here). In the jitcode trampoline, we preallocate space on the stack for the arguments to the comparator function, including this. The intention was that we could save a little bit of time by effectively preallocating part of the call frame, and reusing it across calls.

We already had issues in bug 1888746 reusing the descriptor because we weren't clearing the stale HASCACHEDSAVEDFRAME_BIT. In this case, we're having issues because InvokeFromInterpreterStub, which we call if the comparator function hasn't tiered up yet, stores the return value in argv[0], which is to say the this slot.

We never reset the value of this after initializing it to UndefinedValue, so we end up calling the comparator with this value equal to the return value of the previous call. Most comparators don't use this, but in this case the code is using an explicit this to access global variables.

The best fix is probably to store UndefinedValue on each call, the same way we currently store the callee token.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

:jandem, since you are the author of the regressor, bug 1884360, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 22

[Iain Ireland [:iain]]

Attachment: Bug 1897150: Initialize thisValue on every comparator call r=jandem

I also considered setting this in ArraySortData::setComparatorArgs, but this matches the change we made for the descriptor.

The only data for the comparator call that is still constant is the comparator itself. I think that's still fine, so long as we trace it during a GC, which we do.

Comment 23

[Jan de Mooij [:jandem]]

Thanks Emilio and Iain for tracking this down.

It's a very safe fix that we should consider taking in a dot release.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdafcd6fc8fe
Initialize thisValue on every comparator call r=jandem

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fdafcd6fc8fe

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox127 to wontfix.

For more information, please visit BugBot documentation.

Comment 27

[Donal Meehan [:dmeehan]]

Adding to Comment 26, please include a release uplift request when adding a beta uplift request.
We can include it in a Fx126 dot release

Comment 28

[Iain Ireland [:iain]]

Comment on attachment 9402353 [details]
Bug 1897150: Initialize thisValue on every comparator call r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect results when using this inside a custom comparator function for Array.sort
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very simple patch that writes the value that we need into a stack slot instead of assuming it will be unchanged across calls.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 29

[Pascal Chevrel:pascalc]

Comment on attachment 9402353 [details]
Bug 1897150: Initialize thisValue on every comparator call r=jandem

Approved for 127 beta 5, thanks.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/9aa532725757

Comment 31

[Donal Meehan [:dmeehan]]

Comment on attachment 9402353 [details]
Bug 1897150: Initialize thisValue on every comparator call r=jandem

Approved for 126.0.1

Comment 32

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/cc1e3c686734