New issue
Advanced search Search tips

Issue 715309 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 686281
issue 594639



Sign in to add a comment

Fix prefinalizer chain of ClassicPendingScript

Project Member Reported by hirosh...@chromium.org, Apr 25 2017

Issue description

ClassicPendingScript 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

 
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.

Comment 3 by kouhei@chromium.org, Apr 26 2017

Cc: jbroman@chromium.org
+jbroman FYI
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Now only ClassicScript has a simple prefinalizer that doesn't interact at all with ResourceOwner's prefinalizer.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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