Reland of Split PendingScript into PendingScript and ClassicPendingScript

Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

A unit test for the crash will be added by
https://codereview.chromium.org/2828973002/.

BUG=594639, 686281, 711703
Review-Url: https://codereview.chromium.org/2653923008
Cr-Original-Commit-Position: refs/heads/master@{#464494}
Committed: https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962da97e3bc8ccff
Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#466899}
Committed: https://chromium.googlesource.com/chromium/src/+/77c5ade43bf1d07f035d143ee6f3449d92b5d6ac

[hiroshige, 2017-02-16 00:32:08]

Description was changed from

==========
Schedule module script execution

BUG=
==========

to

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002
https://codereview.chromium.org/2693423002/

BUG=
==========

[hiroshige, 2017-02-22 17:49:33]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002
https://codereview.chromium.org/2693423002/

BUG=
==========

to

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

BUG=
==========

[hiroshige, 2017-02-22 20:07:18]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 20:08:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-02-22 20:19:27]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

BUG=
==========

to

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

imports-noinline.html (kouhei-side issue?):
  FAIL Import a module that tries to import itself assert_unreached: This module
should not have loaded! Reached unreachable code
  FAIL Import a module with a cyclical module dependency assert_unreached: This
module should not have loaded! Reached unreachable code

errorhandling-noinline.html (double error event dispatching? my side):
  FAIL IFrame test: 'iframe_parseError_Dependent' assert_equals: Unexpected
_errorReported value expected "error" but got "error,error"
  FAIL IFrame test: 'iframe_parseError_DependentMultiple' assert_equals:
Unexpected _errorReported value expected "error,error" but got
"error,error,error,error"

crossorigin-noinline.html (kouhei-side issue?):
  Some tests failing (some module not loaded unexpectedly related to CORS?)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[commit-bot: I haz the power, 2017-02-22 20:47:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:47:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[hiroshige, 2017-02-23 00:39:12]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 00:40:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-02-23 00:40:56]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

imports-noinline.html (kouhei-side issue?):
  FAIL Import a module that tries to import itself assert_unreached: This module
should not have loaded! Reached unreachable code
  FAIL Import a module with a cyclical module dependency assert_unreached: This
module should not have loaded! Reached unreachable code

errorhandling-noinline.html (double error event dispatching? my side):
  FAIL IFrame test: 'iframe_parseError_Dependent' assert_equals: Unexpected
_errorReported value expected "error" but got "error,error"
  FAIL IFrame test: 'iframe_parseError_DependentMultiple' assert_equals:
Unexpected _errorReported value expected "error,error" but got
"error,error,error,error"

crossorigin-noinline.html (kouhei-side issue?):
  Some tests failing (some module not loaded unexpectedly related to CORS?)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[commit-bot: I haz the power, 2017-02-23 00:58:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 00:58:14]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hiroshige, 2017-03-27 06:44:06]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org

[hiroshige, 2017-03-27 07:32:05]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 07:32:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 07:39:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 07:39:27]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hiroshige, 2017-03-27 08:15:21]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002

Base Revision: r452109

Status:
- Basic functionality is working, passing some of the tests.
- Test failures:

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002
https://codereview.chromium.org/2774843002

Base Revision: r459714

how to try:
./out/Release/content_shell --run-layout-tests module.html 

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[hiroshige, 2017-03-27 08:16:07]

Description was changed from

==========
Schedule module script execution

Dependencies:
https://codereview.chromium.org/2555653002
https://codereview.chromium.org/2774843002

Base Revision: r459714

how to try:
./out/Release/content_shell --run-layout-tests module.html 

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
Schedule module script execution

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2653923008/ (this CL)

Base Revision: r459714

how to try:
./out/Release/content_shell --run-layout-test module.html 

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[hiroshige, 2017-03-27 08:16:12]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 08:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 08:24:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 08:24:52]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hiroshige, 2017-03-27 09:28:37]

Description was changed from

==========
Schedule module script execution

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2653923008/ (this CL)

Base Revision: r459714

how to try:
./out/Release/content_shell --run-layout-test module.html 

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
Schedule module script execution

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2653923008/ (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[hiroshige, 2017-03-27 09:55:35]

Description was changed from

==========
Schedule module script execution

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2653923008/ (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
Schedule module script execution

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2780463002
4. https://codereview.chromium.org/2653923008 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[hiroshige, 2017-03-28 06:55:45]

Description was changed from

==========
Schedule module script execution

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2780463002
4. https://codereview.chromium.org/2653923008 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

Currently changes are ongoing.
If you want to test, use Patch Set 19 with the following instructions.

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2780463002
4. https://codereview.chromium.org/2653923008 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

[hiroshige, 2017-03-28 07:43:09]

Description was changed from

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

Currently changes are ongoing.
If you want to test, use Patch Set 19 with the following instructions.

How to build:
Apply the following:
1. https://codereview.chromium.org/2555653002
2. https://codereview.chromium.org/2774843002
3. https://codereview.chromium.org/2780463002
4. https://codereview.chromium.org/2653923008 (this CL)

Base Revision: r459714

how to try:
1. Place top-level js/html files somewhere and start a local HTTP server.
2. ./out/Release/content_shell --run-layout-test
http://localhost:8000/module.html 

(file:// urls don't work because they lack Content-Type)

To be splitted into CLs:
- Introduce blink::Script and blink::ClassicScript
- Introduce Classic/ModulePendingScript (this CL)
- Support module execution

BUG=
==========

to

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

See https://codereview.chromium.org/2781713003/ for build/test instructions.

BUG=
==========

[hiroshige, 2017-03-28 07:43:43]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 07:44:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 08:25:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 08:25:42]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[hiroshige, 2017-04-05 22:22:54]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 22:23:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-05 22:51:10]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 22:51:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-05 22:58:55]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 22:59:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-05 23:02:00]

Description was changed from

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

See https://codereview.chromium.org/2781713003/ for build/test instructions.

BUG=
==========

to

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

See https://codereview.chromium.org/2781713003/ for build/test instructions.

BUG=594639, 686281
==========

[hiroshige, 2017-04-05 23:03:12]

Description was changed from

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

See https://codereview.chromium.org/2781713003/ for build/test instructions.

BUG=594639, 686281
==========

to

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281
==========

[hiroshige, 2017-04-05 23:13:52]

hiroshige@chromium.org changed reviewers:
+ sigbjornf@opera.com

[hiroshige, 2017-04-05 23:13:54]

PTAL; Speculatively starting the code review, while some DCHECK()s are still
commented out (and should be handled properly).

[commit-bot: I haz the power, 2017-04-06 00:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 00:24:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2017-04-06 17:51:22]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 17:52:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-06 19:11:49]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org

[commit-bot: I haz the power, 2017-04-06 19:54:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 19:54:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2017-04-07 22:40:11]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[hiroshige, 2017-04-07 22:40:41]

Removed commented-out code by introducing
PendingScript::removeFromMemoryCache().

https://codereview.chromium.org/2653923008/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/2653923008/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:716: //
DCHECK_EQ(pendingScript->resource(), m_resource);
Previously I thought this has a slight chance of failure (e.g. after m_resource
is cleared after execute()) but now I think this is not necessary (after
execute() both m_pendingScript and m_resource are null, causing the DCHECK_EQ()
above to fail).

[commit-bot: I haz the power, 2017-04-07 22:41:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-07 23:21:27]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 23:22:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-08 02:47:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-08 02:47:22]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-10 22:33:30]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 22:34:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 00:45:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 00:45:31]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-12 18:35:15]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 18:36:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 20:43:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 20:43:54]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-13 00:28:24]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-13 00:28:26]

lgtm

[commit-bot: I haz the power, 2017-04-13 00:29:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 00:45:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 00:45:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[haraken, 2017-04-13 01:03:56]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-04-13 01:03:58]

owner LGTM

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ClassicPendingScript.h (right):

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ClassicPendingScript.h:24: class CORE_EXPORT
ClassicPendingScript final

Add a class-level comment and explain what Classic means.

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PendingScript.h:56: class CORE_EXPORT
PendingScript

Add a class-level comment.

[hiroshige, 2017-04-13 17:08:31]

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ClassicPendingScript.h (right):

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ClassicPendingScript.h:24: class CORE_EXPORT
ClassicPendingScript final
On 2017/04/13 01:03:58, haraken wrote:
> 
> Add a class-level comment and explain what Classic means.

Done.

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/2653923008/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PendingScript.h:56: class CORE_EXPORT
PendingScript
On 2017/04/13 01:03:58, haraken wrote:
> 
> Add a class-level comment.

Done.

[hiroshige, 2017-04-13 17:08:44]

Description was changed from

==========
[Module 3] Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281
==========

to

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281
==========

[hiroshige, 2017-04-13 17:08:50]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-04-13 17:08:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2653923008/#ps720001
(title: "Add class-level comments")

[commit-bot: I haz the power, 2017-04-13 17:09:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 19:14:58]

CQ is committing da patch.

Bot data: {"patchset_id": 720001, "attempt_start_ts": 1492103330446500,
"parent_rev": "c028fb577e58a2375ec0f4acc7fa6568feb9f678", "commit_rev":
"d512803299c01324b92ffd8c962da97e3bc8ccff"}

[commit-bot: I haz the power, 2017-04-13 19:15:54]

Description was changed from

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281
==========

to

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[commit-bot: I haz the power, 2017-04-13 19:15:57]

Committed patchset #37 (id:720001) as
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...

[hiroshige, 2017-04-14 18:28:19]

A revert of this CL (patchset #37 id:720001) has been created in
https://codereview.chromium.org/2820753002/ by hiroshige@chromium.org.

The reason for reverting is: Suspected to cause CHECK() failure
(crbug.com/711703)
inside PendingScript's prefinalizer.

BUG=711703, 594639, 686281
.

[hiroshige, 2017-04-14 22:35:35]

Description was changed from

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[hiroshige, 2017-04-14 22:40:37]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 22:41:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-15 00:13:14]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-15 00:13:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-15 00:33:04]

Description was changed from

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL doesn't change the behavior.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[hiroshige, 2017-04-15 00:35:50]

Description was changed from

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL doesn't change the behavior.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[commit-bot: I haz the power, 2017-04-15 01:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-15 01:46:38]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-19 00:35:58]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 00:37:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-19 00:55:06]

Description was changed from

==========
Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

BUG=594639, 686281

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[hiroshige, 2017-04-19 00:56:04]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 00:56:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-19 01:01:32]

Description was changed from

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

This CL also adds a unit test of ClassicPendingScript for Issue 711703.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[hiroshige, 2017-04-19 01:01:40]

PTAL again. The original CL has been reverted due to a prefinalizer-related
crashing issue (Issue 711703) and I made a fix.

Below I highlighted the changes since the committed Patch Set 37.

(Now this CL has more dependencies to add a unit test for this crashing issue
though)

https://codereview.chromium.org/2653923008/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ClassicPendingScript.h (right):

https://codereview.chromium.org/2653923008/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ClassicPendingScript.h:30:
USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
I also register Dispose() as ClassicPendingScript.
(See CL description for the reason).

https://codereview.chromium.org/2653923008/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ClassicPendingScriptTest.cpp (right):

https://codereview.chromium.org/2653923008/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ClassicPendingScriptTest.cpp:60:
TEST(ClassicPendingScript, Prefinalizer) {
I added this unit test (this crashes with Patch Set 40 but doesn't crash with
Patch Set 41).

[commit-bot: I haz the power, 2017-04-19 01:02:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 01:02:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[kouhei (in TOK), 2017-04-19 01:20:42]

lgtm

[hiroshige, 2017-04-19 01:24:28]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 01:24:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 04:53:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 04:53:41]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2017-04-19 22:49:31]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 22:50:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-19 23:50:52]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 23:51:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-19 23:52:08]

On 2017/04/19 01:20:42, kouhei wrote:
> lgtm

Splitted the unit test into https://codereview.chromium.org/2828973002/ and
reverted ScriptStreamerImpl-related changes.

[commit-bot: I haz the power, 2017-04-20 02:07:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 02:07:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2017-04-20 09:40:50]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-04-20 09:40:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2653923008/#ps880001
(title: "comment fix")

[commit-bot: I haz the power, 2017-04-20 09:41:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-20 09:44:18]

Description was changed from

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

This CL also adds a unit test of ClassicPendingScript for Issue 711703.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

A unit test for the crash will be added by
https://codereview.chromium.org/2828973002/.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

[hiroshige, 2017-04-20 09:44:37]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-04-20 09:44:45]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-04-20 09:45:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 15:07:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 15:07:49]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2017-04-20 15:54:35]

Hmm, still failing on windows bots.

1. PendingScript::Dispose() disposes the PendingScript, including its subclass
part (if it is ClassicPendingScript, ClassicPendingScript::DisposeInternal() is
called from Dispose()).
2. I want to call Dispose() for PendingScript and its subclasses at
prefinalization step. So I wrote:
    USING_PRE_FINALIZER(PendingScript, Dispose);
3. For ClassicPendingScript, Dispose() should be called before ResourceOwner's
prefinalizer. Thus I also add:
    USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
because PendingScript's prefinalizer is called after ResourceOwner's
prefinalizer, causing crashes.
4. But this crashes on windows bots, because the two prefinalizers has the same
|this| pointer and the same function pointer:
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...

haraken@, what is the best practice for this case?
(Should we introduce another method for ClassicPendingScript's prefinalizer? Or
instead should we make Dispose() virtual and make ClassicPendingScript to
override it? Or something else?)

[hiroshige, 2017-04-21 17:40:36]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 17:41:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 19:55:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 19:55:49]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2017-04-22 00:41:55]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 00:42:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-22 00:45:19]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 00:45:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 02:45:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-22 02:45:48]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-24 09:50:29]

> Hmm, still failing on windows bots.
> 
> 1. PendingScript::Dispose() disposes the PendingScript, including its subclass
> part (if it is ClassicPendingScript, ClassicPendingScript::DisposeInternal()
is
> called from Dispose()).
> 2. I want to call Dispose() for PendingScript and its subclasses at
> prefinalization step. So I wrote:
>     USING_PRE_FINALIZER(PendingScript, Dispose);
> 3. For ClassicPendingScript, Dispose() should be called before ResourceOwner's
> prefinalizer. Thus I also add:
>     USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
> because PendingScript's prefinalizer is called after ResourceOwner's
> prefinalizer, causing crashes.
> 4. But this crashes on windows bots, because the two prefinalizers has the
same
> |this| pointer and the same function pointer:
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...
> 
> haraken@, what is the best practice for this case?
> (Should we introduce another method for ClassicPendingScript's prefinalizer?
I slightly prefer this over the other.
> (Or
> instead should we make Dispose() virtual and make ClassicPendingScript to
> override it? Or something else?)

[haraken, 2017-04-24 09:57:38]

On 2017/04/24 09:50:29, kouhei wrote:
> > Hmm, still failing on windows bots.
> > 
> > 1. PendingScript::Dispose() disposes the PendingScript, including its
subclass
> > part (if it is ClassicPendingScript, ClassicPendingScript::DisposeInternal()
> is
> > called from Dispose()).
> > 2. I want to call Dispose() for PendingScript and its subclasses at
> > prefinalization step. So I wrote:
> >     USING_PRE_FINALIZER(PendingScript, Dispose);
> > 3. For ClassicPendingScript, Dispose() should be called before
ResourceOwner's
> > prefinalizer. Thus I also add:
> >     USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
> > because PendingScript's prefinalizer is called after ResourceOwner's
> > prefinalizer, causing crashes.
> > 4. But this crashes on windows bots, because the two prefinalizers has the
> same
> > |this| pointer and the same function pointer:
> >
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...
> > 
> > haraken@, what is the best practice for this case?
> > (Should we introduce another method for ClassicPendingScript's prefinalizer?
> I slightly prefer this over the other.
> > (Or
> > instead should we make Dispose() virtual and make ClassicPendingScript to
> > override it? Or something else?)

In general it's not nice to create an ordering dependency between pre-finalizers
(although in practice it's guaranteed that pre-finalizers are called in the
reverse order in which they were registered).

Can we reornigaze the code to remove the ordering dependency?

[hiroshige, 2017-04-24 23:01:28]

On 2017/04/24 09:57:38, haraken wrote:
> On 2017/04/24 09:50:29, kouhei wrote:
> > > Hmm, still failing on windows bots.
> > > 
> > > 1. PendingScript::Dispose() disposes the PendingScript, including its
> subclass
> > > part (if it is ClassicPendingScript,
ClassicPendingScript::DisposeInternal()
> > is
> > > called from Dispose()).
> > > 2. I want to call Dispose() for PendingScript and its subclasses at
> > > prefinalization step. So I wrote:
> > >     USING_PRE_FINALIZER(PendingScript, Dispose);
> > > 3. For ClassicPendingScript, Dispose() should be called before
> ResourceOwner's
> > > prefinalizer. Thus I also add:
> > >     USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
> > > because PendingScript's prefinalizer is called after ResourceOwner's
> > > prefinalizer, causing crashes.
> > > 4. But this crashes on windows bots, because the two prefinalizers has the
> > same
> > > |this| pointer and the same function pointer:
> > >
> >
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...
> > > 
> > > haraken@, what is the best practice for this case?
> > > (Should we introduce another method for ClassicPendingScript's
prefinalizer?
> > I slightly prefer this over the other.
> > > (Or
> > > instead should we make Dispose() virtual and make ClassicPendingScript to
> > > override it? Or something else?)
> 
> In general it's not nice to create an ordering dependency between
pre-finalizers
> (although in practice it's guaranteed that pre-finalizers are called in the
> reverse order in which they were registered).
> 
> Can we reornigaze the code to remove the ordering dependency?

Probably no, and I expect we don't heavily assumes the ordering dependencies.
(kouhei@, please double check)

1. ClassicPendingScript's prefinalizer needs to be called before ResourceOwner's
prefinalizer.
If we call ResourceOwner's prefinalizer before
PendingScript/ClassicPendingScript's prefinalizer, it partially prefinalizes
ClassicPendingScript, leading to a slightly inconsistent state, causing at least
two CHECK()/DCHECK()s to fail.
I expect this assumption of "the prefinalizer of a subclass is called before the
prefinalizer of a base class" is natural.

2. PendingScript also needs a prefinalizer.
Because it needs to call StopWatchingForLoad().

3. No other ordering dependencies (other than 1.) are assumed.
(i.e. no dependencies between PendingScript and ResourceOwner prefinalizers
because ClassicPendingScript's prefinalizer is called first and cleans up
everything)

4. There's a trick to avoid assertion failure on windows:
The latest Patch Set 48 resolves this issue by registering two prefinalizers:
- PendingScript::Dispose() for PendingScript and
- ClassicPendingScript::Dispose() for ClassicPendingScript
that do the same thing.
Making the two prefinalizers to behave differently causes two types of
Dispose()ing, which complicate the code.
NOINLINE is set to ClassicPendingScript::Dispose(), because if we define/declare
Dispose() inline, compiler optimization merges two Dispose()s, and causing
assertion failure on windows bots.

WDYT?

[kouhei (in TOK), 2017-04-25 00:16:28]

On 2017/04/24 23:01:28, hiroshige wrote:
> On 2017/04/24 09:57:38, haraken wrote:
> > On 2017/04/24 09:50:29, kouhei wrote:
> > > > Hmm, still failing on windows bots.
> > > > 
> > > > 1. PendingScript::Dispose() disposes the PendingScript, including its
> > subclass
> > > > part (if it is ClassicPendingScript,
> ClassicPendingScript::DisposeInternal()
> > > is
> > > > called from Dispose()).
> > > > 2. I want to call Dispose() for PendingScript and its subclasses at
> > > > prefinalization step. So I wrote:
> > > >     USING_PRE_FINALIZER(PendingScript, Dispose);
> > > > 3. For ClassicPendingScript, Dispose() should be called before
> > ResourceOwner's
> > > > prefinalizer. Thus I also add:
> > > >     USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
> > > > because PendingScript's prefinalizer is called after ResourceOwner's
> > > > prefinalizer, causing crashes.
> > > > 4. But this crashes on windows bots, because the two prefinalizers has
the
> > > same
> > > > |this| pointer and the same function pointer:
> > > >
> > >
> >
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...
> > > > 
> > > > haraken@, what is the best practice for this case?
> > > > (Should we introduce another method for ClassicPendingScript's
> prefinalizer?
> > > I slightly prefer this over the other.
> > > > (Or
> > > > instead should we make Dispose() virtual and make ClassicPendingScript
to
> > > > override it? Or something else?)
> > 
> > In general it's not nice to create an ordering dependency between
> pre-finalizers
> > (although in practice it's guaranteed that pre-finalizers are called in the
> > reverse order in which they were registered).
> > 
> > Can we reornigaze the code to remove the ordering dependency?
> 
> Probably no, and I expect we don't heavily assumes the ordering dependencies.
> (kouhei@, please double check)
I agree. It would be a non-trivial work to refactor Resource subsys to not
depend on prefinalizers.

> 1. ClassicPendingScript's prefinalizer needs to be called before
ResourceOwner's
> prefinalizer.
> If we call ResourceOwner's prefinalizer before
> PendingScript/ClassicPendingScript's prefinalizer, it partially prefinalizes
> ClassicPendingScript, leading to a slightly inconsistent state, causing at
least
> two CHECK()/DCHECK()s to fail.
> I expect this assumption of "the prefinalizer of a subclass is called before
the
> prefinalizer of a base class" is natural.
> 
> 2. PendingScript also needs a prefinalizer.
> Because it needs to call StopWatchingForLoad().
> 
> 3. No other ordering dependencies (other than 1.) are assumed.
> (i.e. no dependencies between PendingScript and ResourceOwner prefinalizers
> because ClassicPendingScript's prefinalizer is called first and cleans up
> everything)
> 
> 4. There's a trick to avoid assertion failure on windows:
> The latest Patch Set 48 resolves this issue by registering two prefinalizers:
> - PendingScript::Dispose() for PendingScript and
> - ClassicPendingScript::Dispose() for ClassicPendingScript
> that do the same thing.
> Making the two prefinalizers to behave differently causes two types of
> Dispose()ing, which complicate the code.
> NOINLINE is set to ClassicPendingScript::Dispose(), because if we
define/declare
> Dispose() inline, compiler optimization merges two Dispose()s, and causing
> assertion failure on windows bots.
> 
> WDYT?
I think 4 is reasonable for now.

[hiroshige, 2017-04-25 00:31:31]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 00:32:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-25 00:32:46]

On 2017/04/25 00:16:28, kouhei wrote:
> On 2017/04/24 23:01:28, hiroshige wrote:
> > On 2017/04/24 09:57:38, haraken wrote:
> > > On 2017/04/24 09:50:29, kouhei wrote:
> > > > > Hmm, still failing on windows bots.
> > > > > 
> > > > > 1. PendingScript::Dispose() disposes the PendingScript, including its
> > > subclass
> > > > > part (if it is ClassicPendingScript,
> > ClassicPendingScript::DisposeInternal()
> > > > is
> > > > > called from Dispose()).
> > > > > 2. I want to call Dispose() for PendingScript and its subclasses at
> > > > > prefinalization step. So I wrote:
> > > > >     USING_PRE_FINALIZER(PendingScript, Dispose);
> > > > > 3. For ClassicPendingScript, Dispose() should be called before
> > > ResourceOwner's
> > > > > prefinalizer. Thus I also add:
> > > > >     USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
> > > > > because PendingScript's prefinalizer is called after ResourceOwner's
> > > > > prefinalizer, causing crashes.
> > > > > 4. But this crashes on windows bots, because the two prefinalizers has
> the
> > > > same
> > > > > |this| pointer and the same function pointer:
> > > > >
> > > >
> > >
> >
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...
> > > > > 
> > > > > haraken@, what is the best practice for this case?
> > > > > (Should we introduce another method for ClassicPendingScript's
> > prefinalizer?
> > > > I slightly prefer this over the other.
> > > > > (Or
> > > > > instead should we make Dispose() virtual and make ClassicPendingScript
> to
> > > > > override it? Or something else?)
> > > 
> > > In general it's not nice to create an ordering dependency between
> > pre-finalizers
> > > (although in practice it's guaranteed that pre-finalizers are called in
the
> > > reverse order in which they were registered).
> > > 
> > > Can we reornigaze the code to remove the ordering dependency?
> > 
> > Probably no, and I expect we don't heavily assumes the ordering
dependencies.
> > (kouhei@, please double check)
> I agree. It would be a non-trivial work to refactor Resource subsys to not
> depend on prefinalizers.
> 
> > 1. ClassicPendingScript's prefinalizer needs to be called before
> ResourceOwner's
> > prefinalizer.
> > If we call ResourceOwner's prefinalizer before
> > PendingScript/ClassicPendingScript's prefinalizer, it partially prefinalizes
> > ClassicPendingScript, leading to a slightly inconsistent state, causing at
> least
> > two CHECK()/DCHECK()s to fail.
> > I expect this assumption of "the prefinalizer of a subclass is called before
> the
> > prefinalizer of a base class" is natural.
> > 
> > 2. PendingScript also needs a prefinalizer.
> > Because it needs to call StopWatchingForLoad().
> > 
> > 3. No other ordering dependencies (other than 1.) are assumed.
> > (i.e. no dependencies between PendingScript and ResourceOwner prefinalizers
> > because ClassicPendingScript's prefinalizer is called first and cleans up
> > everything)
> > 
> > 4. There's a trick to avoid assertion failure on windows:
> > The latest Patch Set 48 resolves this issue by registering two
prefinalizers:
> > - PendingScript::Dispose() for PendingScript and
> > - ClassicPendingScript::Dispose() for ClassicPendingScript
> > that do the same thing.
> > Making the two prefinalizers to behave differently causes two types of
> > Dispose()ing, which complicate the code.
> > NOINLINE is set to ClassicPendingScript::Dispose(), because if we
> define/declare
> > Dispose() inline, compiler optimization merges two Dispose()s, and causing
> > assertion failure on windows bots.
> > 
> > WDYT?
> I think 4 is reasonable for now.

Thanks. Updated the comments in ClassicPendingScript.h in Patch Set 49
(corresponds to 1. and 4. above).

[hiroshige, 2017-04-25 00:32:54]

On 2017/04/25 00:16:28, kouhei wrote:
> On 2017/04/24 23:01:28, hiroshige wrote:
> > On 2017/04/24 09:57:38, haraken wrote:
> > > On 2017/04/24 09:50:29, kouhei wrote:
> > > > > Hmm, still failing on windows bots.
> > > > > 
> > > > > 1. PendingScript::Dispose() disposes the PendingScript, including its
> > > subclass
> > > > > part (if it is ClassicPendingScript,
> > ClassicPendingScript::DisposeInternal()
> > > > is
> > > > > called from Dispose()).
> > > > > 2. I want to call Dispose() for PendingScript and its subclasses at
> > > > > prefinalization step. So I wrote:
> > > > >     USING_PRE_FINALIZER(PendingScript, Dispose);
> > > > > 3. For ClassicPendingScript, Dispose() should be called before
> > > ResourceOwner's
> > > > > prefinalizer. Thus I also add:
> > > > >     USING_PRE_FINALIZER(ClassicPendingScript, Dispose);
> > > > > because PendingScript's prefinalizer is called after ResourceOwner's
> > > > > prefinalizer, causing crashes.
> > > > > 4. But this crashes on windows bots, because the two prefinalizers has
> the
> > > > same
> > > > > |this| pointer and the same function pointer:
> > > > >
> > > >
> > >
> >
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...
> > > > > 
> > > > > haraken@, what is the best practice for this case?
> > > > > (Should we introduce another method for ClassicPendingScript's
> > prefinalizer?
> > > > I slightly prefer this over the other.
> > > > > (Or
> > > > > instead should we make Dispose() virtual and make ClassicPendingScript
> to
> > > > > override it? Or something else?)
> > > 
> > > In general it's not nice to create an ordering dependency between
> > pre-finalizers
> > > (although in practice it's guaranteed that pre-finalizers are called in
the
> > > reverse order in which they were registered).
> > > 
> > > Can we reornigaze the code to remove the ordering dependency?
> > 
> > Probably no, and I expect we don't heavily assumes the ordering
dependencies.
> > (kouhei@, please double check)
> I agree. It would be a non-trivial work to refactor Resource subsys to not
> depend on prefinalizers.
> 
> > 1. ClassicPendingScript's prefinalizer needs to be called before
> ResourceOwner's
> > prefinalizer.
> > If we call ResourceOwner's prefinalizer before
> > PendingScript/ClassicPendingScript's prefinalizer, it partially prefinalizes
> > ClassicPendingScript, leading to a slightly inconsistent state, causing at
> least
> > two CHECK()/DCHECK()s to fail.
> > I expect this assumption of "the prefinalizer of a subclass is called before
> the
> > prefinalizer of a base class" is natural.
> > 
> > 2. PendingScript also needs a prefinalizer.
> > Because it needs to call StopWatchingForLoad().
> > 
> > 3. No other ordering dependencies (other than 1.) are assumed.
> > (i.e. no dependencies between PendingScript and ResourceOwner prefinalizers
> > because ClassicPendingScript's prefinalizer is called first and cleans up
> > everything)
> > 
> > 4. There's a trick to avoid assertion failure on windows:
> > The latest Patch Set 48 resolves this issue by registering two
prefinalizers:
> > - PendingScript::Dispose() for PendingScript and
> > - ClassicPendingScript::Dispose() for ClassicPendingScript
> > that do the same thing.
> > Making the two prefinalizers to behave differently causes two types of
> > Dispose()ing, which complicate the code.
> > NOINLINE is set to ClassicPendingScript::Dispose(), because if we
> define/declare
> > Dispose() inline, compiler optimization merges two Dispose()s, and causing
> > assertion failure on windows bots.
> > 
> > WDYT?
> I think 4 is reasonable for now.

Thanks. Updated the comments in ClassicPendingScript.h in Patch Set 49
(corresponds to 1. and 4. above).

[hiroshige, 2017-04-25 00:46:17]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 00:46:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kouhei (in TOK), 2017-04-25 01:53:34]

lgtm

[commit-bot: I haz the power, 2017-04-25 02:57:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 02:57:12]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-25 06:24:47]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-04-25 06:24:47]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2653923008/#ps980001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-25 06:25:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 06:29:37]

CQ is committing da patch.

Bot data: {"patchset_id": 980001, "attempt_start_ts": 1493101487209500,
"parent_rev": "63575c0e1d7b89a2658f34a07a6bb7ba3a3d1e97", "commit_rev":
"77c5ade43bf1d07f035d143ee6f3449d92b5d6ac"}

[commit-bot: I haz the power, 2017-04-25 06:29:53]

Description was changed from

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

A unit test for the crash will be added by
https://codereview.chromium.org/2828973002/.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
==========

to

==========
Reland of Split PendingScript into PendingScript and ClassicPendingScript

This is preparation for introducing ModulePendingScript.

This CL shouldn't change the behavior.

This has been reverted due to crashing (Issue 711703) because
ResourceOwner's prefinalizer is called before PendingScript's prefinalizer,
causing CheckState() assertion failure.
This reland fixes this issue by registering PendingScript::Dispose() also
as the prefinalizer of ClassicPendingScript, which is called before
ResourceOwner's prefinalizer.

A unit test for the crash will be added by
https://codereview.chromium.org/2828973002/.

BUG=594639, 686281, 711703

Review-Url: https://codereview.chromium.org/2653923008
Cr-Original-Commit-Position: refs/heads/master@{#464494}
Committed:
https://chromium.googlesource.com/chromium/src/+/d512803299c01324b92ffd8c962d...
Review-Url: https://codereview.chromium.org/2653923008
Cr-Commit-Position: refs/heads/master@{#466899}
Committed:
https://chromium.googlesource.com/chromium/src/+/77c5ade43bf1d07f035d143ee6f3...
==========

[commit-bot: I haz the power, 2017-04-25 06:29:56]

Committed patchset #50 (id:980001) as
https://chromium.googlesource.com/chromium/src/+/77c5ade43bf1d07f035d143ee6f3...

[haraken, 2017-04-25 17:19:51]

https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp (right):

https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:61: void
ClassicPendingScript::DisposeInternal() {

Why can't you call DisposeInternal() before ClassicPendingScript::Dispose()
calls PendingScript::Dispose()?

I'm concerned that this CL is introducing a pretty complicated pre-finalizer
chain. You're doing something like:

1) Call ClassisPendingScript's Dispose()
2) Call PendingScript's Dispose()
3) Let PendingScript's Dispose() call ClassisPendingScript's DisposeInternal()

In the first place, I'm concerned that the resource hierarchy is doing a lot of
complex work in the pre-finalizer. The pre-finalizer is designed to do a couple
of only simple things (e.g., clearing raw pointer). For example, it will cause a
use-after-free bug if the pre-finalizer adds a new reference to an already dead
object (because the marking phase has already completed). Are you pretty sure
that the Resource's pre-finalizer is not doing such a thing?

I'd strongly recommend you refactor the Resource's hierarchy so that it doesn't
heavily rely on the pre-finalizers.

[haraken, 2017-04-25 17:20:37]

On 2017/04/25 17:19:51, haraken wrote:
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp (right):
> 
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:61: void
> ClassicPendingScript::DisposeInternal() {
> 
> Why can't you call DisposeInternal() before ClassicPendingScript::Dispose()
> calls PendingScript::Dispose()?
> 
> I'm concerned that this CL is introducing a pretty complicated pre-finalizer
> chain. You're doing something like:
> 
> 1) Call ClassisPendingScript's Dispose()
> 2) Call PendingScript's Dispose()
> 3) Let PendingScript's Dispose() call ClassisPendingScript's DisposeInternal()
> 
> In the first place, I'm concerned that the resource hierarchy is doing a lot
of
> complex work in the pre-finalizer. The pre-finalizer is designed to do a
couple
> of only simple things (e.g., clearing raw pointer). For example, it will cause
a
> use-after-free bug if the pre-finalizer adds a new reference to an already
dead
> object (because the marking phase has already completed). Are you pretty sure
> that the Resource's pre-finalizer is not doing such a thing?
> 
> I'd strongly recommend you refactor the Resource's hierarchy so that it
doesn't
> heavily rely on the pre-finalizers.

Due to the excessive complexity of the pre-finalizers, I'd say not LGTM.

[hiroshige, 2017-04-25 22:46:30]

On 2017/04/25 17:20:37, haraken wrote:
> On 2017/04/25 17:19:51, haraken wrote:
> >
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:61: void
> > ClassicPendingScript::DisposeInternal() {
> > 
> > Why can't you call DisposeInternal() before ClassicPendingScript::Dispose()
> > calls PendingScript::Dispose()?
> > 
> > I'm concerned that this CL is introducing a pretty complicated pre-finalizer
> > chain. You're doing something like:
> > 
> > 1) Call ClassisPendingScript's Dispose()
> > 2) Call PendingScript's Dispose()
> > 3) Let PendingScript's Dispose() call ClassisPendingScript's
DisposeInternal()
> > 
> > In the first place, I'm concerned that the resource hierarchy is doing a lot
> of
> > complex work in the pre-finalizer. The pre-finalizer is designed to do a
> couple
> > of only simple things (e.g., clearing raw pointer). For example, it will
cause
> a
> > use-after-free bug if the pre-finalizer adds a new reference to an already
> dead
> > object (because the marking phase has already completed). Are you pretty
sure
> > that the Resource's pre-finalizer is not doing such a thing?
> > 
> > I'd strongly recommend you refactor the Resource's hierarchy so that it
> doesn't
> > heavily rely on the pre-finalizers.
> 
> Due to the excessive complexity of the pre-finalizers, I'd say not LGTM.

I'll address the issue and start codereview for fixing this shortly
(crbug.com/715309).

[hiroshige, 2017-04-26 00:38:26]

On 2017/04/25 22:46:30, hiroshige wrote:
> On 2017/04/25 17:20:37, haraken wrote:
> > On 2017/04/25 17:19:51, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:61: void
> > > ClassicPendingScript::DisposeInternal() {
> > > 
> > > Why can't you call DisposeInternal() before
ClassicPendingScript::Dispose()
> > > calls PendingScript::Dispose()?
> > > 
> > > I'm concerned that this CL is introducing a pretty complicated
pre-finalizer
> > > chain. You're doing something like:
> > > 
> > > 1) Call ClassisPendingScript's Dispose()
> > > 2) Call PendingScript's Dispose()
> > > 3) Let PendingScript's Dispose() call ClassisPendingScript's
> DisposeInternal()
> > > 
> > > In the first place, I'm concerned that the resource hierarchy is doing a
lot
> > of
> > > complex work in the pre-finalizer. The pre-finalizer is designed to do a
> > couple
> > > of only simple things (e.g., clearing raw pointer). For example, it will
> cause
> > a
> > > use-after-free bug if the pre-finalizer adds a new reference to an already
> > dead
> > > object (because the marking phase has already completed). Are you pretty
> sure
> > > that the Resource's pre-finalizer is not doing such a thing?
> > > 
> > > I'd strongly recommend you refactor the Resource's hierarchy so that it
> > doesn't
> > > heavily rely on the pre-finalizers.
> > 
> > Due to the excessive complexity of the pre-finalizers, I'd say not LGTM.
> 
> I'll address the issue and start codereview for fixing this shortly
> (crbug.com/715309).

Created CLs (crbug.com/715309) that address haraken's not-l-g-t-m:
https://codereview.chromium.org/2837363003/
https://codereview.chromium.org/2839033002/
https://codereview.chromium.org/2844583002/

[hiroshige, 2017-04-26 16:17:40]

On 2017/04/26 00:38:26, hiroshige wrote:
> On 2017/04/25 22:46:30, hiroshige wrote:
> > On 2017/04/25 17:20:37, haraken wrote:
> > > On 2017/04/25 17:19:51, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2653923008/diff/980001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:61: void
> > > > ClassicPendingScript::DisposeInternal() {
> > > > 
> > > > Why can't you call DisposeInternal() before
> ClassicPendingScript::Dispose()
> > > > calls PendingScript::Dispose()?
> > > > 
> > > > I'm concerned that this CL is introducing a pretty complicated
> pre-finalizer
> > > > chain. You're doing something like:
> > > > 
> > > > 1) Call ClassisPendingScript's Dispose()
> > > > 2) Call PendingScript's Dispose()
> > > > 3) Let PendingScript's Dispose() call ClassisPendingScript's
> > DisposeInternal()
> > > > 
> > > > In the first place, I'm concerned that the resource hierarchy is doing a
> lot
> > > of
> > > > complex work in the pre-finalizer. The pre-finalizer is designed to do a
> > > couple
> > > > of only simple things (e.g., clearing raw pointer). For example, it will
> > cause
> > > a
> > > > use-after-free bug if the pre-finalizer adds a new reference to an
already
> > > dead
> > > > object (because the marking phase has already completed). Are you pretty
> > sure
> > > > that the Resource's pre-finalizer is not doing such a thing?
> > > > 
> > > > I'd strongly recommend you refactor the Resource's hierarchy so that it
> > > doesn't
> > > > heavily rely on the pre-finalizers.
> > > 
> > > Due to the excessive complexity of the pre-finalizers, I'd say not LGTM.
> > 
> > I'll address the issue and start codereview for fixing this shortly
> > (crbug.com/715309).
> 
> Created CLs (crbug.com/715309) that address haraken's not-l-g-t-m:
> https://codereview.chromium.org/2837363003/
> https://codereview.chromium.org/2839033002/
> https://codereview.chromium.org/2844583002/

The follow-up fixes have been landed.