Looks like the CSS Resource gets collected once pi is detached and pi.sheet is null until the CSS Resource finishes loading.

It realiable reproduces on that revision with stand-alone debug content shell and incremental marking (incl stress) flag turned on.

Feel free to pick it up if you think that you know how to debug this in an efficient way. Otherwise, I will pick it up sometime next week.

It could be a good way to figure out what's going on with resource loading and GC.

This test expects
1. CSS Resource finish loading (NotifyFinished()) sets sheet_
2. Removing pi from document (RemovedFrom()) clears sheet_
3. Inserting the pi back will synchronously set sheet_ because the Resource is in MemoryCache

But when a GC occurs before onload...
1. CSS Resource finish loading (NotifyFinished()) sets sheet_
2. GC collects CSS Resource
3. Removing pi from document (RemovedFrom()) clears sheet_
4. Inserting the pi back does not set sheet_ because the Resource is not yet loaded
5. Test fails because pi.sheet returns null

You could add gc() to the first line of the onload handler to make this test fail on non-incremental marking builds. So this is not an incremental marking specific issue.

According to the spec, ProcessingInstruction should implement the same LinkStyle interface as <link type="text/css">.
https://drafts.csswg.org/cssom/#requirements-on-user-agents-implementing-the-xml-stylesheet-processing-instruction

I confirmed that the same thing will happen with <link type="text/css"> instead of xml stylesheet. So this is not a xml stylesheet specific issue.

I propose we fix the test to wait for pi.sheet to be available.

> I propose we fix the test to wait for pi.sheet to be available.

I might be misunderstanding but doesn't it mean that a GC behavior is exposed to a user script?

MemoryCache does not hold Resources strongly. Maybe should we keep the Resource alive in another way?

Creating a strong reference to an unnecessary object doesn't seem ideal.

The root of the issue is MemoryCache holds weak references so you can do a timing attack to detect GC. The issue you pointed out is that, this attack is too easy because the attribute changes synchronously/asynchronously with Resource load and you don't need a timer. This is not a ProcessingInstruction.sheet only problem as I just confirmed that you can do the same thing with Image.naturalWidth/Height. Everything that uses ResourceClient::NotifyFinished could have this problem so I don't think this is fixable.

Thanks, I got your point.

+loading experts

fyi, there are a couple of tests failing when rigorously performing GCs at random at when reaching the event loop (I am thinking about making this a stress mode).

They could all be related in a way as they deal with resource loading/caching, I think. Non-exhaustive list:
- fast/dom/StyleSheet/detached-style-pi.xhtml
- fast/dom/StyleSheet/detached-style-2.html
- inspector-protocol/css/css-coverage-poll.js
- http/tests/devtools/startup/cached-resource-metadata.js

Maybe issue 831541 is also related?

Ping, can we get an estimate of what it would take to fix this? 

This should already flake on ToT and is currently blocking any more rigorous GC testing.

I want to get feedback from loading experts. kinuko-san or kouhei-san?

(Note: This is a week of national holidays in Japan, so let’s wait for the reply until next week:-)

Oh, sorry that I missed that. Happy golden week!

The NextAction date has arrived: 2018-05-07

kinoku or kouhei, could you have a look? thanks :)

I chatted with haraken@ off thread.

Summary:
- Making user JS execution non-deterministic because of GC is discouraged, since they confuse developers.
- MemoryCache references being weak && Fetches finishing synchronously when MemoryCache hit can make user JS execution non-deterministic.
- kouhei@ thinks that the long-term solution is to make MC-hit fetches return asynchronously, but the change is non-trivial since it is expected to introduce performance impact.

I'd propose to modify the tests so that they don't expose this nondeterminism for now.

This is not an incremental marking issue but a general GC/loading issue.

kinuko & kouhei: If the plan is to work it around by modifying tests, would it be possible to handle it in the loading team?