Replace GetCycleRoot with [[CycleRoot]] field

[guybedford]

This fixes a spec bug first reported by @sokra in #158 (comment) and being tracked in V8 in https://bugs.chromium.org/p/v8/issues/detail?id=11376.

The same example case is described in both links above.

The issue as discovered by @syg is that when GetCycleRoot(C2) is called, the execution of C2 has already happened sync, so it does not participate in the AsyncParentModules linkage being utilized by GetCycleRoot. As a result X isn't properly depending on the root of the cycle at C1 and is instead depending directly on the execution of C2 which will have been completed already.

The fix is to ensure GetCycleRoot gives the proper strongly connected component root corresponding to the DFSAncestorIndex. A naive adjustment would be to iterate through the imported modules down the DFSIndex to reach this, but that approach would risk interactions with other DFS numberings which would result in non-deterministic behaviour. If we did use a global DFS numbering that would be one way to work around that, but it seems unnecessary.

For this reason I've instead simply added a new field to link the cycle root reference for each module directly, and setting this in the main cycle transition code path. It comes out quite simply from that.

Review would be much appreciated further.

[devsnek]

has this change been validated in implementation yet? I can get around to merging it into engine262 eventually but it might be good for someone to do that proactively

[codehag]

I am validating it now, We also found the same issue in our implementation and I have a test for it.

[syg]

TLA ships in Chrome 89 and stable branch cut for Chrome 89 is on Feb 23. I'm not sure this is a high-enough profile patch to warrant a back merge into the branch from tip-of-tree, but I'll ask.

[codehag]

This does indeed fix the issue raised in #156 and I can also now say with certainty that with the previous spec we were not hitting line 12.d.iv.3 -- even with the test @jorendorff wrote. It just looked like there was an issue in our cycle implementation, but we verified that we had the spec pretty much exactly implemented. I believe this is a required change, albeit minor, and we will likely need to bring it up as an update at the next meeting.

With the change in this patch, this cycle issue is fixed. All of the test262 tests, along with our local tests, pass. We should incorporate the test written in #156 into test262 -- though it is masked by #152 -- I will see if I can get a test that will fail regardless..

Implementation: https://phabricator.services.mozilla.com/D103808

[codehag]

This looks good and fixes the issue.

[syg]

The fix is easy to implement in V8 and decreases code complexity at the cost of an extra field per SourceTextModule (now called Cyclic Module Records in the spec) object. The extra memory isn't an issue today but if we ever want to approach a world with thousands of ESMs per page, we might need to take another look.

V8 CL here: https://chromium-review.googlesource.com/c/v8/v8/+/2667772

[syg]

lgtm with nits

[littledan]

I haven't quite traced the bug exactly, but the new logic is a lot more straightforward, so LGTM based on that. I noticed what may be a typo, though.