Bug 172575

Summary: Properties set on window.customElements can disappear due to GC
Product: WebKit Reporter: Russell Bicknell <bicknellr>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, dbates, esprehn+autocc, ews-watchlist, fpizlo, fred.wang, kangil.han, kondapallykalyan, mark.lam, rniwa, saam, sam, tsavell, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari 10   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 154907, 188353    
Attachments:
Description Flags
test.html
none
index.html
none
test.html
none
Fixes the bug saam: review+

Description Russell Bicknell 2017-05-24 20:35:52 PDT
Occasionally, properties that are set on window.customElements very early in the lifetime of the page are removed by garbage collection. To repro, put the attached files in the same directory and open 'index.html'. You may need to adjust the number of objects created in 'test.html' to balance the repro rate with the time per run. You can also trigger this (at a reduced rate) by putting an external script that spins for 10-20ms between the write / read instead of creating objects. This seems to affect 10, TP, and Nightly.
Comment 1 Russell Bicknell 2017-05-24 20:36:19 PDT
Created attachment 311187 [details]
test.html
Comment 2 Russell Bicknell 2017-05-24 20:36:37 PDT
Created attachment 311188 [details]
index.html
Comment 3 Russell Bicknell 2017-05-24 20:38:54 PDT
Comment on attachment 311187 [details]
test.html

<script>
  // Uncomment this section to prevent the bug.
  /*
  Object.defineProperty(window, 'customElements', {
    value: window.customElements,
  });
  */

  window.customElements.prop = 1234;

  const a = [];
  for (let i = 0; i < 1000000; i++) {
    a.push({});
  }

  const pass = window.customElements.prop === 1234;
  window.parent.postMessage(pass, '*');
</script>
Comment 4 Russell Bicknell 2017-05-24 20:39:27 PDT
Created attachment 311189 [details]
test.html
Comment 5 Russell Bicknell 2017-05-24 20:46:31 PDT
This seems like it's probably related to 171567.
Comment 6 Radar WebKit Bug Importer 2017-05-27 00:13:35 PDT
<rdar://problem/32440668>
Comment 7 Ryosuke Niwa 2017-05-27 00:22:43 PDT
(In reply to Russell Bicknell from comment #5)
> This seems like it's probably related to 171567.

It's not.
Comment 8 Ryosuke Niwa 2018-08-03 21:13:14 PDT
Created attachment 346581 [details]
Fixes the bug
Comment 9 Ryosuke Niwa 2018-08-04 02:02:46 PDT
Committed r234578: <https://trac.webkit.org/changeset/234578>
Comment 10 Darin Adler 2018-08-05 15:15:04 PDT
Comment on attachment 346581 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=346581&action=review

> Source/WebCore/dom/CustomElementRegistry.idl:29
>      EnabledAtRuntime=CustomElements,
> -    ImplementationLacksVTable,
>      JSGenerateToNativeObject,
> +    GenerateIsReachable=ImplScriptExecutionContext

We’ve been sorting these alphabetically and putting commas on every line. So this should be a line higher and have a comma.
Comment 11 Ryosuke Niwa 2018-08-05 17:45:14 PDT
Sure. Fixed it in r234585.
Comment 12 Truitt Savell 2018-08-06 09:38:20 PDT
Looks like the new test fast/custom-elements/custom-element-registry-wrapper-should-stay-alive.html

is a constant timeout on all debug platforms. 

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fcustom-elements%2Fcustom-element-registry-wrapper-should-stay-alive.html
Comment 13 Ryosuke Niwa 2018-08-06 10:20:41 PDT
Yeah, noticed. Will look into it today.
Comment 14 Ryosuke Niwa 2018-08-06 13:30:26 PDT
The test failure is tracked by https://bugs.webkit.org/show_bug.cgi?id=188353.