Fix prefinalizer chain of ClassicPendingScript |
|||
Issue descriptionClassicPendingScript and its two parent classes (PendingScript and ResourceOwner) have prefinalizers and should be cleaned up. In fact I expect we can simplify/remove prefinalizers so that we only call ScriptStreamer::Cancel() there. doc: https://docs.google.com/document/d/1FyQRB64y5Af093p1wfZHIKDeeIXvt4bIefXvR-EYlF4/edit
,
Apr 25 2017
Created draft CLs: 1. https://codereview.chromium.org/2837413002/ 2. https://codereview.chromium.org/2837363003/ 3. https://codereview.chromium.org/2839033002/ 4. https://codereview.chromium.org/2844583002/ (Splitted into small CLs to make bisect easier) Waiting for bot results.
,
Apr 26 2017
+jbroman FYI
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f72cff34bada6f453c14170273e2c67354209109 commit f72cff34bada6f453c14170273e2c67354209109 Author: hiroshige <hiroshige@chromium.org> Date: Wed Apr 26 02:12:32 2017 Check that ClassicPendingScript is not accessed after prefinalizer explicitly To ensure upcoming changes to prefinalizers around PendingScript, this CL asserts that ClassicPendingScript is not touched after prefinalization. The state transition of ClassicPendingScript and ScriptStreamer is complicated, and I'd like to check that our change doesn't break the their assumptions. BUG= 715309 Review-Url: https://codereview.chromium.org/2837413002 Cr-Commit-Position: refs/heads/master@{#467205} [modify] https://crrev.com/f72cff34bada6f453c14170273e2c67354209109/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp [modify] https://crrev.com/f72cff34bada6f453c14170273e2c67354209109/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d70ed2630a00fee3b5014b015ec765155885e54 commit 3d70ed2630a00fee3b5014b015ec765155885e54 Author: hiroshige <hiroshige@chromium.org> Date: Wed Apr 26 03:56:53 2017 Remove HTMLParserScriptRunner's prefinalizer We don't have to Dispose() PendingScripts, because: - If HTMLParserScriptRunner is watching for load of a PendingScript, then the PendingScript has a Member<HTMLParserScriptRunner> of |this|, and thus at prefinalization the PendingScript is also to be garbage collected. Dispose() is called by PendingScript's prefinalizer. - Otherwise, what matters in PendingScript::Dispose() is ScriptStreamer::Cancel() in ClassicPendingScript::DisposeInternal(). This CL might defer that ScriptStreamer::Cancel() to when ClassicPendingScript is prefinalized, but this shouldn't cause anything functionally wrong. BUG= 715309 Review-Url: https://codereview.chromium.org/2837363003 Cr-Commit-Position: refs/heads/master@{#467224} [modify] https://crrev.com/3d70ed2630a00fee3b5014b015ec765155885e54/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp [modify] https://crrev.com/3d70ed2630a00fee3b5014b015ec765155885e54/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00a279559130cd2818c66eaf047a77d39158858a commit 00a279559130cd2818c66eaf047a77d39158858a Author: hiroshige <hiroshige@chromium.org> Date: Wed Apr 26 04:48:41 2017 Remove PendingScript's prefinalizer We don't have to clear |client_| and |element_| because they are Member<>, and also we can left other members of PendingScript as we can leave the object in a potentially corrupted state after prefinalization. Also, after https://codereview.chromium.org/2837363003/, PendingScript seems not touched during other prefinalizers. BUG= 715309 Review-Url: https://codereview.chromium.org/2839033002 Cr-Commit-Position: refs/heads/master@{#467235} [modify] https://crrev.com/00a279559130cd2818c66eaf047a77d39158858a/third_party/WebKit/Source/core/dom/PendingScript.h
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/faa055103d806be6bcdb05b721c954d1b1584200 commit faa055103d806be6bcdb05b721c954d1b1584200 Author: hiroshige <hiroshige@chromium.org> Date: Wed Apr 26 11:44:51 2017 Do not call Dispose() as ClassicPendingScript's prefinalizer Because the things in ClassicPendingScript::DisposeInternal() except for ScriptStreamer::Cancel() doesn't have to be called as a prefinalizer, this CL introduces ClassicPendingScript::Prefinalize() that only calls ScriptStreamer::Cancel() and thus makes Dispose() not to be called there. This CL simplified the prefinalization of ClassicPendingScript, especially order dependencies between ClassicPendingScript's prefinalizer and the prefinalizer of its parent class (ResourceOwner). Leaving ClassicPendingScript in a not-Dispose()d state is not observable if the related classes obeys the rule of Oilpan, and https://codereview.chromium.org/2837413002/ checks that in case there were a bug. BUG= 715309 Review-Url: https://codereview.chromium.org/2844583002 Cr-Commit-Position: refs/heads/master@{#467299} [modify] https://crrev.com/faa055103d806be6bcdb05b721c954d1b1584200/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp [modify] https://crrev.com/faa055103d806be6bcdb05b721c954d1b1584200/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
,
Apr 26 2017
Now only ClassicScript has a simple prefinalizer that doesn't interact at all with ResourceOwner's prefinalizer.
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e541a6eaee202a358f9f746f797096f51f0fc4cc commit e541a6eaee202a358f9f746f797096f51f0fc4cc Author: Leszek Swirski <leszeks@chromium.org> Date: Tue Sep 25 13:11:33 2018 [blink] Move ClassicPendingScript prefinalizer to ScriptStreamer The prefinalizer in CPS no longer does anything except for cancelling the ScriptStreamer. Since ScriptStreamer could operate independently of the CPS, we can simply cancel the ScriptStreamer in its own prefinalizer. Bug: 715309 Change-Id: I5866f510bea62d633390e247eac33653a0f30a71 Reviewed-on: https://chromium-review.googlesource.com/1242466 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Leszek Swirski <leszeks@chromium.org> Cr-Commit-Position: refs/heads/master@{#593908} [modify] https://crrev.com/e541a6eaee202a358f9f746f797096f51f0fc4cc/third_party/blink/renderer/bindings/core/v8/script_streamer.cc [modify] https://crrev.com/e541a6eaee202a358f9f746f797096f51f0fc4cc/third_party/blink/renderer/bindings/core/v8/script_streamer.h [modify] https://crrev.com/e541a6eaee202a358f9f746f797096f51f0fc4cc/third_party/blink/renderer/core/script/classic_pending_script.cc [modify] https://crrev.com/e541a6eaee202a358f9f746f797096f51f0fc4cc/third_party/blink/renderer/core/script/classic_pending_script.h
,
Oct 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ed3b3c26c8366db8fc93bbd57b30f97fa79a7e5 commit 1ed3b3c26c8366db8fc93bbd57b30f97fa79a7e5 Author: Leszek Swirski <leszeks@chromium.org> Date: Wed Oct 03 10:44:00 2018 [config] Disable scheduled script streaming field trial Measure the impact of the current trial by seeing what regressed, and disable while waiting for Bug:874080 to be resolved, since there are currently some loading regressions. Bug: 715309 Bug: 885053 Bug: 886668 Change-Id: I4497e11373ffa92d3e5f5844fa9fbe73fef6bf4b Reviewed-on: https://chromium-review.googlesource.com/c/1255946 Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Reviewed-by: Steven Holte <holte@chromium.org> Commit-Queue: Leszek Swirski <leszeks@chromium.org> Cr-Commit-Position: refs/heads/master@{#596175} [modify] https://crrev.com/1ed3b3c26c8366db8fc93bbd57b30f97fa79a7e5/testing/variations/fieldtrial_testing_config.json
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83ec360fbb41f7b81a628417b1aa5f13daaa622d commit 83ec360fbb41f7b81a628417b1aa5f13daaa622d Author: Leszek Swirski <leszeks@chromium.org> Date: Wed Nov 21 16:50:55 2018 Revert "[config] Disable scheduled script streaming field trial" This reverts commit 1ed3b3c26c8366db8fc93bbd57b30f97fa79a7e5. Reason for revert: Re-enabling the trial now that 874080 is being field tested. Original change's description: > [config] Disable scheduled script streaming field trial > > Measure the impact of the current trial by seeing what regressed, and > disable while waiting for Bug:874080 to be resolved, since there are > currently some loading regressions. > > Bug: 715309 > Bug: 885053 > Bug: 886668 > Change-Id: I4497e11373ffa92d3e5f5844fa9fbe73fef6bf4b > Reviewed-on: https://chromium-review.googlesource.com/c/1255946 > Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> > Reviewed-by: Steven Holte <holte@chromium.org> > Commit-Queue: Leszek Swirski <leszeks@chromium.org> > Cr-Commit-Position: refs/heads/master@{#596175} TBR=rmcilroy@chromium.org,holte@chromium.org,leszeks@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 715309 , 885053, 886668 Change-Id: I59f9e0d1f5b97a75d7d81e98471cd21a3a68888b Reviewed-on: https://chromium-review.googlesource.com/c/1296502 Reviewed-by: Steven Holte <holte@chromium.org> Commit-Queue: Leszek Swirski <leszeks@chromium.org> Cr-Commit-Position: refs/heads/master@{#610107} [modify] https://crrev.com/83ec360fbb41f7b81a628417b1aa5f13daaa622d/testing/variations/fieldtrial_testing_config.json |
|||
►
Sign in to add a comment |
|||
Comment 1 by hirosh...@chromium.org
, Apr 25 2017