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

Issue 596018 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Assertion failure in ElementAnimations on Android Release Build

Project Member Reported by drott@chromium.org, Mar 18 2016

Issue description

Version: ToT, Monday 03/14, Git bc71ad0
OS: Android

What steps will reproduce the problem?
Build a GN android release build
GN args:
  target_os = "android"
  is_clang = true
Run the attached HTML test case

Observe crash with Assertion failure.

03-18 11:08:41.192  4289  4304 W WebKit  : ASSERTION FAILED: *m_baseComputedStyle == *computedStyle
03-18 11:08:41.192  4289  4304 W WebKit  : ../../third_party/WebKit/Source/core/animation/ElementAnimations.cpp(128) : void blink::ElementAnimations::updateBaseComputedStyle(const blink::ComputedStyle *)
--------- beginning of crash
03-18 11:08:41.192  4289  4304 F libc    : Fatal signal 11 (SIGSEGV), code 1, fault addr 0xfbadbeef in tid 4304 (CrRendererMain)
03-18 11:08:41.253   368   368 I SELinux : SELinux: Loaded file_contexts contexts from /file_contexts.
03-18 11:08:41.254   368   368 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
03-18 11:08:41.255   368   368 F DEBUG   : Build fingerprint: 'google/shamu/shamu:6.0.1/MMB29K/2419427:user/release-keys'
03-18 11:08:41.255   368   368 F DEBUG   : Revision: '0'
03-18 11:08:41.255   368   368 F DEBUG   : ABI: 'arm'
03-18 11:08:41.255   368   368 F DEBUG   : pid: 4289, tid: 4304, name: CrRendererMain  >>> org.chromium.chrome:sandboxed_process0 <<<
03-18 11:08:41.255   368   368 F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xfbadbeef
03-18 11:08:41.268   368   368 F DEBUG   :     r0 fbadbeef  r1 00000000  r2 00004001  r3 00000001
03-18 11:08:41.268   368   368 F DEBUG   :     r4 2e0af448  r5 26aec6f8  r6 4eb04198  r7 b380abd8
03-18 11:08:41.268   368   368 F DEBUG   :     r8 b380b344  r9 2e0af448  sl 00000000  fp 32263588
03-18 11:08:41.268   368   368 F DEBUG   :     ip 938dba6d  sp b380ab80  lr 9511ee1d  pc 9511ee26  cpsr 60070030

Now reproduced on a public build, created a new issue report for this after the previous one was on an internal build.
 
emoji-seq-anim.html
939 bytes View Download

Comment 1 by drott@chromium.org, Mar 18 2016

dstockwell@'s previous comment: 
"If this is related to emoji then very likely font related -- probably an issue similar to https://bugs.chromium.org/p/chromium/issues/detail?id=470458"

Comment 2 by drott@chromium.org, Mar 18 2016

Cc: -r...@opera.com mikelawther@chromium.org
mikelawther@ believes this should be looked at by font people. But before I dig deeper, rune@, would you have any idea about this one, since dstockwell@ points out you've worked on a similar issue, changing font-cache update triggers.


Comment 3 by drott@chromium.org, Mar 18 2016

Cc: -dstockwell@chromium.org r...@opera.com

Comment 4 by drott@chromium.org, Mar 18 2016

Cc: dstockwell@chromium.org

Comment 5 by r...@opera.com, Mar 18 2016

drott@, yes, the general problem is around fonts and operator== for ComputedStyle.

baseLayoutStyle is an optimization we have where we during an animation find out that the only style changes happening for an element is for an ongoing animation. We then store the ComputedStyle for the element without animations applied, and for every animation frame clone that computed style applying animations on top. Thus, we avoid doing selector matching and style application for the rest of the computed style.

With asserts enabled, we store the baseLayoutStyle, but do the full styleForElement to check that the optimization is sound. I've been through various fixes for ComputedStyle::operator==() where it would return false when the computed style conceptually is the same while there were some flags/members which were different, yet the optimization was sound.

Some of the font comparisons have been moved out of operator== and compared explicitly outside of that operator in places like comparing loading status for custom fonts [1].

I can not reproduce in Linux content_shell. Have you been able to reproduce with an android build?

I have just started to fetch for doing an android build.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/dom/Element.cpp&q=Element::recalcOw&sq=package:chromium&type=cs&l=1633

Comment 6 by drott@chromium.org, Mar 18 2016

Thanks for looking into this and your explanations. 

So far I was only able to reproduce this on Android. Because of  issue 555855  I can only reproduce color emoji issues on Linux when I do a release build. On Android the assertions are enabled, on Linux they aren't. 

Looking into gn args --list, I will now try using dcheck_always_on = true on the Linux release build and try to reproduce there.

Comment 7 Deleted

Comment 8 by drott@chromium.org, Mar 18 2016

Unfortunately, I can't reproduce this on Linux even with "dcheck_always_on = true" and having verified that the #if ENABLE(ASSERT) parts of ElementAnimations.cpp are compiled in.

Comment 9 by r...@opera.com, Mar 21 2016

Owner: r...@opera.com
Status: Assigned (was: Available)
I've made a debug content shell apk which triggers this. Looking now.

Comment 10 by r...@opera.com, Mar 21 2016

Text auto sizing is changing the computed style under our feet.

Comment 11 by r...@opera.com, Mar 21 2016

Status: Started (was: Assigned)
Up for review: https://codereview.chromium.org/1816103002
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 21 2016

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

commit 32a7b19e92a9ff0fca3aba45c1fdd3896fdd5f1d
Author: rune <rune@opera.com>
Date: Mon Mar 21 19:17:22 2016

Clear baseComputedStyle when text-autosizing changes.

baseComputedStyle is an optimization for animations where the computed
style before animations are applied is cached and cloned to have
cheaper style recalcs for per-frame animation changes. An assumption is
that the computed style for the layout object only changes in
styleForElement or pseudoStyleForElement. That assumption is not true
for text autosizing as the computed style may be changed during layout.
Then, for the next animation frame, the text autosizing factor may be
different even though the style has not been marked for recalc, and the
sanity check for an unchanged baseComputedStyle will trigger an assert.

Make sure we clear the baseComputedStyle for an element when the text
autosizing factor changes.

R=pdr@chromium.org,drott@chromium.org
BUG= 596018 

Review URL: https://codereview.chromium.org/1816103002

Cr-Commit-Position: refs/heads/master@{#382350}

[add] https://crrev.com/32a7b19e92a9ff0fca3aba45c1fdd3896fdd5f1d/third_party/WebKit/LayoutTests/fast/text-autosizing/basecomputedstyle-assert-expected.txt
[add] https://crrev.com/32a7b19e92a9ff0fca3aba45c1fdd3896fdd5f1d/third_party/WebKit/LayoutTests/fast/text-autosizing/basecomputedstyle-assert.html
[modify] https://crrev.com/32a7b19e92a9ff0fca3aba45c1fdd3896fdd5f1d/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/32a7b19e92a9ff0fca3aba45c1fdd3896fdd5f1d/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/32a7b19e92a9ff0fca3aba45c1fdd3896fdd5f1d/third_party/WebKit/Source/core/layout/TextAutosizer.cpp

Comment 13 by drott@chromium.org, Mar 22 2016

Status: Fixed (was: Started)
Thanks!

Sign in to add a comment