New issue
Advanced search Search tips

Issue 808047 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 177475



Sign in to add a comment

DocumentLifecycle.cpp has a fallthrough that looks unintentional

Project Member Reported by thakis@chromium.org, Feb 1 2018

Issue description

https://codereview.chromium.org/2795263002/diff/140001/third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp added a case to a switch that doesn't end in break or a "falling through" comment. I think the break is probably missing and unintentional.

The only case where it makes a difference is for next_state == kInPrePait.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 1 2018

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

commit 06100b277e237deebdd47cb6c3465ec345c57608
Author: Nico Weber <thakis@chromium.org>
Date: Thu Feb 01 18:03:57 2018

Enable -Wimplicit-fallthrough on release non-chromecast non-ozone linux builds.

clang will now warn on non-empty case and default blocks that don't end
with a break statement.  For cases where fallthrough is intentional,
end the block with FALLTHROUGH; from base/compiler-specific

Depends on these changes to DEPS rolling into chromium:
https://pdfium-review.googlesource.com/#/c/pdfium/+/24790
https://chromium-review.googlesource.com/c/angle/angle/+/895728
https://chromium-review.googlesource.com/c/v8/v8/+/895704
https://chromium-review.googlesource.com/c/breakpad/breakpad/+/895705
https://github.com/google/cld3/pull/9

We'll enable this for more build configurations once they can build
with the warning enabled.

(Also change a single file that was needed to get the dcheck_always_on
build to work, and another one that is built only in static builds.)

Bug:  177475 , 808047 
Change-Id: Ic6a3fbf78a9cb2b5e928ce7973e67e4a39629dfd
Reviewed-on: https://chromium-review.googlesource.com/895726
Reviewed-by: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533729}
[modify] https://crrev.com/06100b277e237deebdd47cb6c3465ec345c57608/build/config/compiler/BUILD.gn
[modify] https://crrev.com/06100b277e237deebdd47cb6c3465ec345c57608/third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp
[modify] https://crrev.com/06100b277e237deebdd47cb6c3465ec345c57608/tools/ipc_fuzzer/fuzzer/mutator.cc

Status: Started (was: Assigned)
Definitely not intended, thanks for catching! I'll put up a CL to fix it and add a test if I can.
Blocking: 177475

Comment 4 by kochi@chromium.org, Feb 2 2018

Components: -Blink Blink>DOM
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 2 2018

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

commit d205c1a266c494be54f368a0d05317ce159ab587
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Feb 02 21:18:50 2018

Fix missing 'break' in DocumentLifeCycle::CanAdvanceTo

No test is added because this is a DHCECK_IS_ON only method.

Bug:  808047 
Change-Id: I615a07aec4223dd4c029f6e59dc864ffc7a0f739
Reviewed-on: https://chromium-review.googlesource.com/897053
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534154}
[modify] https://crrev.com/d205c1a266c494be54f368a0d05317ce159ab587/third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp

Status: Fixed (was: Started)

Sign in to add a comment