1812508 - Unexpected behaviour with can.js library causing circular references

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Ksenia Berezina [:ksenia]]

Attachment: 116642.zip

We've received a report in https://github.com/webcompat/web-bugs/issues/116642 where some elements are not rendered correctly on https://manage.opensrs.com. The site is using can.js library.

While the site requires login, I've came up with a slightly reduced test case that javascript code on the site and attached an archive with sample code (can be run with python3 -m http.server 8000).

To reproduce:

1. Open 116642.html in release (109) or recent Nightly
2. Scroll to the bottom of the page and observe the list towards the end

Actual:
[object Object] is displayed (as item.title here is a reference to item)

Expected:
Titles are displayed

This looks like a timing issue since adding debugger statements makes it work correctly. It doesn't happen in Chrome or Firefox prior to bug1798365. Also it depends on the size/contents of the processed array.

The library does custom processing of arrays/objects to create observable objects. I did some analysis and added logs to the library source code to try and debug it.

The objects are stored in "cache" (basically another object) with a certain key and this cache is cleared periodically. At some point of looping through the list of items a key becomes undefined and an item is stored in cache with that key, which is I believe is causing the circular reference (during some later processing/deep copy) as it keeps referring to the same object in cache until it's cleared again.

Comment 1

[Ksenia Berezina [:ksenia]]

There is the following log :
adding to cache with property: undefined cid: ..., which I put inside the addToMap function:

	addToMap: function addToMap(obj, instance) {
		var teardown;

		// Setup a fresh mapping if `madeMap` is missing.
		if (!madeMap) {
			teardown = teardownMap;
			madeMap = {};
		}

		// Record if Object has a `_cid` before adding one.
		var hasCid = obj._cid;
		var cid = canCid_1_3_1_canCid(obj);


		// Only update if there already isn't one already.
		if (!madeMap[cid]) {

            if (!cid) {
                console.log('adding to cache with property:', cid, 'cid:', obj._cid)
            }

			madeMap[cid] = {
				obj: obj,
				instance: instance,
				added: !hasCid
			};
		}
		return teardown;
	},

It's unclear why var cid = canCid_1_3_1_canCid(obj); is undefined , as it's clearly defined, coming from this function:

var cid = function (object, name) {
	var propertyName = object.nodeName ? domExpando : "_cid";

	if (!object[propertyName]) {
		_cid++;
		object[propertyName] = (name || '') + _cid;
	}

	return object[propertyName];
};

FWIW, if I put if(!object[propertyName]) console.log(object[propertyName]) right before return, it returns a proper value and everything works as expected.

Comment 2

[Ksenia Berezina [:ksenia]]

Hi Doug, wonder if you could take a look at this, please?

Comment 3

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

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

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1812508 - Set dependency of result in MMegamorphicLoadSlotByValue::foldsTo r?jandem

I assumed that this would have been covered in ValueNumbering.cpp, but it
appears this is a one-of-a-kind foldsTo, in that every other one either folds
to an existing node or to a constant?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Can we try to land a test for this too?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecc39a833ede
Set dependency of result in MMegamorphicLoadSlotByValue::foldsTo r=jandem

Comment 7

[Norisz Fay [:noriszfay]]

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

Comment 8

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

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

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

For more information, please visit auto_nag documentation.

Comment 9

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 9314589 [details]
Bug 1812508 - Set dependency of result in MMegamorphicLoadSlotByValue::foldsTo r?jandem

Beta/Release Uplift Approval Request

• User impact if declined: Unknown. Some JS in the wild will not execute correctly. I would speculate that this is hit very very rarely, as a lot of details have to line up to hit it, but ultimately we don't know.
• 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): The patch is quite minimal, and all it does is set a dependency on a MIR node which was previously null, so in the unlikely event that something about the patch is incorrect, it should only make the JIT's output more conservative, not less.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9314589 [details]
Bug 1812508 - Set dependency of result in MMegamorphicLoadSlotByValue::foldsTo r?jandem

Approved for 110.0b9.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b1f3b27cddaf

Comment 12

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the issue by following the STR from comment 0 with Firefox 111.0a1 (2023-01-25) on Windows 10x64. After opening the attached test case [object Object] strings are displayed at the bottom of the page.
The issue is verified fixed with Firefox 111.0a1 (2023-02-02) and 110.0b9 on Windows x64, macOS 12, and Ubuntu 20.04. The [object Object] strings are no longer displayed at the bottom of the page.