Closed Bug 1899734 Opened 6 months ago Closed 2 months ago

SharedSubResourceCache doesn't create corresponding PerformanceEntry

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
133 Branch
Tracking Flags:
Tracking Status
firefox133 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: parity-chrome, parity-safari)

Attachments

(9 files)

Bug 1899734 - Part 1: Move SharedSubResourceCache::CompleteSubResource class definition before SharedSubResourceCache::Result. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 2: Add constructors to SharedSubResourceCache::Result. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 3: Add a constructor to SharedSubResourceCache::CompleteSubResource. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 4: Add SheetLoadData::InitiatorTypeString method. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 5: Make SharedSubResourceCache capable of handling CacheablePerformanceTimingData. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 6: Set SheetLoadData::mLoadStart also when coalesced. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 7: Carry SubResourceNetworkMetadataHolder with SheetLoadData for complete and incomplete load, and propagate through dependencies. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 8: Add performance entry when stylesheet is created from a cache. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1899734 - Part 9: Add tests. r?emilio!
48 bytes, text/x-phabricator-request
Details | Review

(this requires 2 files for testcase, so I'm not attaching it)

Steps to reproduce:

  1. prepare local server (e.g. python3 -m http.server 8000) providing the following 2 files
  2. run Nightly 128.0a1 (2024-05-30) (64-bit) on macOS
  3. open http://localhost:8000/test.html
  4. open Web Console
  5. Reload the page with F5

Actual result:

  • 1 is logged at steps 3-4
  • 0 is logged at step 5

Expected result:
The same number is loggged between steps 3-4 and step 5.

Testcase:

  • test.html
<html>
<head>
<title>test</title>
<script>
function onLoad() {
  console.log(performance.getEntriesByType("resource").length);
}
</script>
<link rel="stylesheet" href="test.css" onload="onLoad()">
</head>
<body>
Hello
</body>
</html>
  • test.css
body { color: red }

Some observation:

Chrome and Safari shows 1 for both case.

This is caused by SharedStyleSheetCache (SharedSubResourceCache) or its consumer not creating corresponding PerformanceEntry when the cache is used.
This issue disappears when mozilla::SharedStyleSheetCache::InsertIfNeeded is modified to immediately return, so that it doesn't cache anything.

void SharedStyleSheetCache::InsertIfNeeded(css::SheetLoadData& aData) {
  return;
...

I'm still not sure how strict the requirement for PerformanceEntry existence is tho, at least there's behavior difference between browsers.

This also affects JS case when used for ScriptLoader in bug 1896709.
https://treeherder.mozilla.org/jobs?repo=try&revision=c92897a6d6bd58d7f6c7524bb3d25915b26306cf&selectedTaskRun=bl6GOEdiS_auHbrpHu5wyw.0

The following testcase can hit the CSS issue by rewriting it to use CSS instead of JS, with ensuring sufficient expiration time for the CSS file.
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/resource-timing/load-from-mem-cache-transfer-size.html

Keywords: parity-chrome, parity-safari

So far, the following spec steps seem to be relevant.

From fetch to adding entry

https://fetch.spec.whatwg.org/#fetch-finale

The fetch response handover, given
  a fetch params fetchParams and a response response,
run these steps:
...
  3. Let processResponseEndOfBody be the following steps:
    ...
    3. Set fetchParams’s controller’s report timing steps to the following steps given a global object global:
      ...
      8. If fetchParams’s request’s initiator type is non-null, then
         mark resource timing given
           timingInfo, fetchParams’s request’s URL,
           fetchParams’s request’s initiator type, global, cacheState, bodyInfo,
           and responseStatus.

https://www.w3.org/TR/resource-timing/#dfn-mark-resource-timing

To mark resource timing given
  a fetch timing info timingInfo, a DOMString requestedURL,
  a DOMString initiatorType a global object global, a string cacheMode,
  a response body info bodyInfo, a status responseStatus,
  and an optional string deliveryType (by default, the empty string),
perform the following steps:

  1. Create a PerformanceResourceTiming object entry in global's realm.
  2. Setup the resource timing entry for entry, given
       initiatorType, requestedURL, timingInfo, cacheMode, bodyInfo,
       responseStatus, and deliveryType.
  3. Queue entry.
  4. Add entry to global's performance entry buffer.

https://w3c.github.io/resource-timing/#sec-performanceresourcetiming

[Exposed=(Window,Worker)]
interface PerformanceResourceTiming : PerformanceEntry {
  ...
};

https://www.w3.org/TR/resource-timing/#dfn-setup-the-resource-timing-entry

To setup the resource timing entry for PerformanceResourceTiming entry given
  DOMString initiatorType, DOMString requestedURL,
  fetch timing info timingInfo, a DOMString cacheMode,
  a response body info bodyInfo, a status responseStatus,
  and an optional DOMString deliveryType (by default, the empty string),
perform the following steps:

   ...
   3. Initialize entry given
        the result of converting timingInfo's start time given global,
        "resource", requestedURL,
        and the result of converting timingInfo's end time given global.

https://www.w3.org/TR/performance-timeline/#dfn-initialize-a-performanceentry

To initialize a PerformanceEntry entry given
  a DOMHighResTimeStamp startTime, a DOMString entryType, a DOMString name,
  and an optional DOMHighResTimeStamp endTime (default 0):

  ...
  3. Initialize entry's entryType to entryType.

https://www.w3.org/TR/performance-timeline/#dfn-queue-a-performanceentry

To queue a PerformanceEntry (newEntry), run these steps:

  ...
  3. Let entryType be newEntry’s entryType value.
  ...
  9. Let tuple be the relevant performance entry tuple of
       entryType and relevantGlobal.
  ...
  11. Let shouldAdd be the result of should add entry with newEntry as input.
  12. If isBufferFull is false and shouldAdd is true,
      append newEntry to tuple's performance entry buffer.

https://www.w3.org/TR/performance-timeline/#dfn-relevant-performance-entry-tuple

In order to get the relevant performance entry tuple, given
  entryType and globalObject as input,
run the following steps:

  1. Let map be the performance entry buffer map associated with globalObject.
  2. Return the result of getting the value of an entry from map, given
       entryType as the key.

https://www.w3.org/TR/performance-timeline/#performance-timeline

Each global object has:
  ...
  * a performance entry buffer map map, keyed on a DOMString,
    representing the entry type to which the buffer belongs.
    The map's value is the following tuple:
      * A performance entry buffer to store PerformanceEntry objects,
        that is initially empty.
      ...

https://w3c.github.io/timing-entrytypes-registry/#dfn-should-add-entry

  * Each entry type must specify an algorithm should add entry which, given
      a PerformanceEntry entry of the entry type and
      optionally a PerformanceObserverInit options of an observer observing
      that entry type,
    determines the following:

      1. If options was not provided, whether entry is eligible to be added to
         the performance timeline, regardless of limitations in the buffer size.
      2. Otherwise, given registered performance observer regObs containing
          options in its options list, whether entry must be appended to
          regObs's observer's observer buffer.

Here, I cannot find the definition of "should add entry" algorithm for "resource".
If it somehow allows omitting the entry, then the current behavior might be consiered spec-compliant, but if not, then the entry should be added.

Getting entry

https://www.w3.org/TR/performance-timeline/#getentriesbyname-method

3.1.3 getEntriesByName() method

Returns a PerformanceEntryList object returned by
filter buffer map by name and type algorithm with
  name set to the method input name parameter,
  and type set to null if optional entryType is omitted,
  or set to the method's input type parameter otherwise.

https://www.w3.org/TR/performance-timeline/#dfn-filter-buffer-map-by-name-and-type

When asked to run the filter buffer map by name and type algorithm with
  optional name and type,
run the following steps:

...
  2. Let map be the performance entry buffer map associated with the relevant
     global object of this.
  3. Let tuple list be an empty list.
  4. If type is not null,
     append the result of getting the value of entry on map given
       type as key to tuple list.
     Otherwise, assign the result of get the values on map to tuple list.
  5. For each tuple in tuple list, run the following steps:
    1. Let buffer be tuple's performance entry buffer.
    ...
    3. Let entries be the result of running filter buffer by name and type with
       buffer, name and type as inputs.
    4. For each entry in entries, append entry to result.
  ...
  7. Return result.

emilio, can I have your input?
could this be a problem for CSS cache?

Flags: needinfo?(emilio)

I don't know what the expectation of the performance API with regards to in-memory caching. My guess is that at most we should dispatch it only once per document. Does this reproduce for images as well?

In general... If there's no fetch, it makes some amount of sense that there's no performance entry and such. That said it seems that test explicitly expects the in-memory-cache to create a PerformanceEntry (from the test name).

Sean do you happen to know off-hand what other browsers do here and if the spec says something about in-memory caching?

Thanks.

Flags: needinfo?(emilio) → needinfo?(sefeng)

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

Does this reproduce for images as well?

I observe the same issue with image.
performance.getEntriesByType("resource") returns non-empty array (1 entry) only for the initial load, and empty array for reloads

And Chrome and Safari always return non-empty array (1 entry)

Here, I cannot find the definition of "should add entry" algorithm for "resource".

It refers to this table https://w3c.github.io/timing-entrytypes-registry/#registry

So I didn't dig into the spec...but I know in general we do want to report entries when cache is being used. WebPerf Working Group often discusses issues about what should this/that attributes be when cache is being used for Resource Timing APIs, so I am fairly certain that we should report an entry here, especially given Chrome and Safari also do this.

Flags: needinfo?(sefeng)
Severity: -- → S3
Priority: -- → P3

Thank you for the info.

So, this means the current CSS and image behavior doesn't match the spec, and we need to fix this before introducing the SharedSubResourceCache to JS in bug 1896709.

Blocks: 1896709
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
See Also: → 1914120
Depends on: 1916601
Blocks: 1916635
See Also: → 1916960
Blocks: 1915626
Attachment #9426831 - Attachment description: Bug 1899734 - Part 4: Add Loader::InitiatorType type and add it to SheetLoadData field. r?emilio! → Bug 1899734 - Part 4: Add SheetLoadData::InitiatorTypeString method. r?emilio!
Depends on: 1921110
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/autoland/rev/a2928c130a54 Part 1: Move SharedSubResourceCache::CompleteSubResource class definition before SharedSubResourceCache::Result. r=emilio https://hg.mozilla.org/integration/autoland/rev/20108e454971 Part 2: Add constructors to SharedSubResourceCache::Result. r=emilio https://hg.mozilla.org/integration/autoland/rev/432c1fa3fbcc Part 3: Add a constructor to SharedSubResourceCache::CompleteSubResource. r=emilio https://hg.mozilla.org/integration/autoland/rev/baa464d5f255 Part 4: Add SheetLoadData::InitiatorTypeString method. r=emilio https://hg.mozilla.org/integration/autoland/rev/313c1f782833 Part 5: Make SharedSubResourceCache capable of handling CacheablePerformanceTimingData. r=emilio https://hg.mozilla.org/integration/autoland/rev/a15f046e5cdf Part 6: Set SheetLoadData::mLoadStart also when coalesced. r=emilio https://hg.mozilla.org/integration/autoland/rev/b59dcea26b32 Part 7: Carry SubResourceNetworkMetadataHolder with SheetLoadData for complete and incomplete load, and propagate through dependencies. r=emilio https://hg.mozilla.org/integration/autoland/rev/4778a482abd6 Part 8: Add performance entry when stylesheet is created from a cache. r=emilio https://hg.mozilla.org/integration/autoland/rev/e960d1241302 Part 9: Add tests. r=emilio
Regressions: 1925778
QA Whiteboard: [qa-133b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: