New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 696775 link

Starred by 2 users
Status: Started
Owner:
hirosh...@chromium.org
Cc:
kouhei@chromium.org
█ sigbjo...@opera.com
jbroman@chromium.org
neis@chromium.org
hirosh...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 695730
Blocking:
issue 686281



Sign in to add a comment

Fix cancellation handling in HTMLParserScriptRunner::pendingScriptFinished()

Project Member Reported by hirosh...@chromium.org, Feb 27 2017 
In HTMLParserScriptRunner::pendingScriptFinished(), when the PendingScript is cancelled during executing a script, we simply dispose()s the PendingScript.

[A] https://codereview.chromium.org/2693423002 introduced a bug, because after this CL we have to clear pointers to PendingScript, not just calling PendingScript::dispose(). This regression is tracked e.g. by  Issue 695730 .

[B] However, there looks still a subtle bug:
pendingScriptFinished() can be called in two cases, where
(1) |pendingScript| is |m_parserBlockingScript|, or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|,
corresponding to two call sites of watchForLoad().
In the case of (2), PendingScript is silently disposed without calling
HTMLParserScriptRunner::executeScriptsWaitingForParsing(), and thus I suspect HTMLDocumentParser::end() is not called in some situations.
I think this is not a recent regression.

I'm not so sure whether/how frequently the case (2) occurs (so far clusterfuzz or I haven't found any test case), or the issue [B] actually matters.

There was a code block that handles the case (2) explicitly, so perhaps the case (2) actually occurs?:
Before https://codereview.chromium.org/2549143009,
HTMLParserScriptRunner::stopWatchingResourceForLoad() handles both cases.

My plan is to fix [A] to fix recent regressions, and handle [B] later.
Also I'll add a CHECK() for a short period of time (only on canary, or DCHECK()) that causes the case (2) to crash, expecting clusterfuzz to find a nice test case for that.

Does anyone know anything about these cases?
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a08766e7f8ec19cc09341d18b91107377ccfbcd6

commit a08766e7f8ec19cc09341d18b91107377ccfbcd6
Author: hiroshige <hiroshige@chromium.org>
Date: Tue Feb 28 10:28:05 2017

Clear pointers to PendingScript when dispose()d in pendingScriptFinished()

In HTMLParserScriptRunner, |pendingScript| is dispose()d but the
corresponding pointer to it is not cleared and thus causing the use of
PendingScript that is already dispose()d,
causing CHECK() failure in PendingScript::element().

There can be two cases, where
(1) |pendingScript| is |m_parserBlockingScript|
    (Clusterfuzz found a test case at  Issue 695730 ), or
(2) |pendingScript| is |m_scriptsToExecuteAfterParsing.first()|
    (no test case found so far).

This CL fixes these cases by clearing |m_parserBlockingScript| or
removing |m_scriptsToExecuteAfterParsing.first()|.

This CL also adds CHECK(false) for (2), which should be removed shortly,
hoping clusterfuzz find a test case for (2).

This is a regression since https://codereview.chromium.org/2693423002.
Before that, when |m_parserBlockingScript| was already disposed, then
it was considered as not having a parser blocking script, and thus
calling PendingScript::dispose() was sufficient.

BUG= 695730 , 696775, 686281

Review-Url: https://codereview.chromium.org/2720683003
Cr-Commit-Position: refs/heads/master@{#453564}

[modify] https://crrev.com/a08766e7f8ec19cc09341d18b91107377ccfbcd6/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
Project Member

Comment 2 by bugdroid1@chromium.org, May 21 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c365bd25d9c0c2d847bc7cc5aab378ed0d29e1c9

commit c365bd25d9c0c2d847bc7cc5aab378ed0d29e1c9
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon May 21 12:26:03 2018

retire CHECK(false)

Bug:  695730 , 696775, 686281
Change-Id: Id11515ceb7ce61fa15411a57ac4a105d03f3e6e6
Reviewed-on: https://chromium-review.googlesource.com/1065559
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560258}
[modify] https://crrev.com/c365bd25d9c0c2d847bc7cc5aab378ed0d29e1c9/third_party/blink/renderer/core/script/html_parser_script_runner.cc
►

Sign in to add a comment