Bug 189018 - ActiveDOMObjects should be GC collectable as soon as their document is being stopped
Summary: ActiveDOMObjects should be GC collectable as soon as their document is being ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-27 15:33 PDT by youenn fablet
Modified: 2021-11-01 12:52 PDT (History)
6 users (show)

See Also:


Attachments
WIP (16.40 KB, patch)
2018-08-27 15:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-sierra (2.99 MB, application/zip)
2018-08-27 17:30 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews202 for win-future (12.81 MB, application/zip)
2018-08-27 18:37 PDT, EWS Watchlist
no flags Details
Patch (7.89 KB, patch)
2018-09-05 10:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews203 for win-future (12.75 MB, application/zip)
2018-09-05 13:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (12.94 MB, application/zip)
2018-09-05 14:37 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-08-27 15:33:19 PDT
This applies currently to WebKitMediaSession, IDBDatabase, IDBRequest, MediaDevices and ActiveDOMObjects using setPendingActivity/unsetPendingActivity.
Comment 1 youenn fablet 2018-08-27 15:36:30 PDT
Created attachment 348217 [details]
WIP
Comment 2 EWS Watchlist 2018-08-27 15:43:33 PDT
Comment on attachment 348217 [details]
WIP

Attachment 348217 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9001574

New failing tests:
(JS) JSTestInterface.cpp
(JS) JSTestNamedConstructor.cpp
Comment 3 EWS Watchlist 2018-08-27 17:30:52 PDT
Comment on attachment 348217 [details]
WIP

Attachment 348217 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9002727

Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2018-08-27 17:30:54 PDT
Created attachment 348243 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-08-27 18:37:09 PDT
Comment on attachment 348217 [details]
WIP

Attachment 348217 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9003673

New failing tests:
http/tests/navigation/page-cache-xhr.html
http/tests/xmlhttprequest/xhr-response-constructor-subframe.html
Comment 6 EWS Watchlist 2018-08-27 18:37:20 PDT
Created attachment 348249 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 7 Geoffrey Garen 2018-08-28 10:09:09 PDT
Comment on attachment 348217 [details]
WIP

What happens in the back-forward cache? Is the document considered stopped in that case? If so, how do you avoid GC?
Comment 8 Chris Dumez 2018-08-28 10:33:43 PDT
(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 348217 [details]
> WIP
> 
> What happens in the back-forward cache? Is the document considered stopped
> in that case? If so, how do you avoid GC?

No, state is suspended while in page cache, not stopped.
Comment 9 Chris Dumez 2018-08-28 10:35:08 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Geoffrey Garen from comment #7)
> > Comment on attachment 348217 [details]
> > WIP
> > 
> > What happens in the back-forward cache? Is the document considered stopped
> > in that case? If so, how do you avoid GC?
> 
> No, state is suspended while in page cache, not stopped.

As in we call suspend() on the active dom objects before entering page cache, never stop(). Stop() is only called when the script execution context is about to be destroyed.
Comment 10 Chris Dumez 2018-08-28 10:46:13 PDT
Comment on attachment 348217 [details]
WIP

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

> Source/WebCore/dom/ScriptExecutionContext.h:133
> +    virtual bool isStopped() = 0;

There is already ScriptExecutionContext::activeDOMObjectsAreStopped()
Comment 11 youenn fablet 2018-09-05 10:37:25 PDT
Created attachment 348937 [details]
Patch
Comment 12 EWS Watchlist 2018-09-05 13:40:22 PDT
Comment on attachment 348937 [details]
Patch

Attachment 348937 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9104756

New failing tests:
http/tests/navigation/page-cache-xhr.html
http/tests/xmlhttprequest/xhr-response-constructor-subframe.html
Comment 13 EWS Watchlist 2018-09-05 13:40:36 PDT
Created attachment 348960 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 14 EWS Watchlist 2018-09-05 14:37:19 PDT
Comment on attachment 348937 [details]
Patch

Attachment 348937 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9105570

New failing tests:
http/tests/navigation/page-cache-xhr.html
http/tests/xmlhttprequest/xhr-response-constructor-subframe.html
Comment 15 EWS Watchlist 2018-09-05 14:37:31 PDT
Created attachment 348963 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 16 Alex Christensen 2021-11-01 12:52:38 PDT
Comment on attachment 348937 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.