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

Issue 594646 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

[S7 SM-G930F] Crash on search or on loading any URL

Project Member Reported by aska...@chromium.org, Mar 14 2016

Issue description

Application Version (from "Chrome Settings > About Chrome"): 50.0.2661.32 (64-bit build)
Android Build Number (from "Android Settings > About Phone/Tablet"): MRA58K
Device: Samsung galaxy S7 (SM_G930F)
Is Merge Tabs and Apps enabled: OFF

Steps to reproduce: 
1. Launch Chrome
2. DO some search from omnibox or navigate to some URL (Ex. cnn.com)

Observed behavior: 
Renderer crash.
Report id: https://crash.corp.google.com/browse?stbtiq=ee01dea800000000 

Expected behavior: 
No crash

Frequency: 
100% 

Bisect range:
Good build: 50.0.2638.3 
Bad Build: 50.0.2639.0

Additional comments: 
* Doesn't crash with 32-bit apk.
* Doesn't crash on S7 verizon
 

Comment 2 by aluo@chromium.org, Mar 14 2016

I also see this issue while testing webview.  The trigger seems to be different since browsing a webpage will work fine for a few seconds then crash will happen.   Have attached the logs.  I'm using webview shell browser for testing.
s7int50.0.2661.32.log
511 KB View Download

Comment 3 by amin...@google.com, Mar 14 2016

Cc: hablich@chromium.org
Labels: -M-50 M-51
I'm swapping this from an M50 blocker to an M51 blocker.  We've already regressed the dev channel with M50, and our last planned M50 dev release is tomorrow, where we have no chance of getting a fix in time.  We also don't see the build in 32-bit builds, and we don't ship 64-bit builds to beta and stable, so a fix for 51 should be fine.

M51 dev is planned for Thursday, so we should ensure we have a fix landed on trunk by Wednesday.
That is the same range as https://bugs.chromium.org/p/chromium/issues/detail?id=593867 and it also looks the same. Please verify that this does not work with an officially build 50.0.2661.32 (v8:5.0.71.16). If you really can reproduce it with that specific V8 version (5.0.71.16) can you please bisect the V8 range https://chromium.googlesource.com/v8/v8/+log/4.10.127..4.10.139? We don't have an S7 device so we cannot have a look ourselve. How to bisect into V8:

Preparation
-Checkout Chromium branch
-Verify that it builds
-Verify that the error shows
-Modify ./DEPS and set it to the first GOOD V8 revision (4.10.127)
-gclient sync
-verify that ./v8/ points to 4.10.127
-Verify that the error does not show

Than repeat until culprit CL found:
1.) Set V8 revision in ./DEPS to V8 revision used for bisect
2.) gclient sync
3.) Test
4.) Note result

Comment 5 by amin...@google.com, Mar 15 2016

[Bulk edit]

This issue is marked as a dev blocker for Android M51.  We will be cutting the candidate build for our first M51 dev tomorrow evening, which means a CL to fix this issue must be landed on trunk by tomorrow @ 3-4 PM PT.  Please prioritize fixing this issue ASAP.

If you don't feel this issue should block the release, please remove the ReleaseBlock-Dev label.  If you have other questions or concerns, please let me know by looping me in to the bug.
Labels: -ReleaseBlock-Dev
Since this is not a regression, there's no point in letting it block releases.

As #4 noted, the regression range and crash pattern look identical to the bug we just fixed. Which is weird, because over the course of finding that fix I looked at crash reports, and didn't see any coming from SM-G930F and exhibiting the same pattern (SIGBUS with pc=0x439da9017b9d). Further, I was under the impression that local testing confirmed the fix for G930V/G930P.

It will be very interesting to see how 50.0.2661.32 behaves in the field when it rolls out.

In the meantime, the log in #2 provided an interesting clue: The value in register x16 == ip is 0x9100439da9017b9d, which is almost the same as the crashing pc except for the leading "91", so clearly we did a jump to the value in "ip" (which is not uncommon). When interpreted as instructions, this value disassembles to:

9D 7B 01 A9  --  stp x29, x30, [x28,#16]
9D 43 00 91  --  add x29, x28, #0x10

which is generated by MacroAssembler::EmitFrameSetupForCodeAgePatching:

  __ stp(fp, lr, MemOperand(jssp, 2 * kXRegSize));
  __ add(fp, jssp, StandardFrameConstants::kFixedFrameSizeFromFp);

These two instructions are in the place where aged code puts a jump target, so this bug is quite clearly related to code aging and i-cache flushing.

The fact that we have been able to fix issue 593867 indicates that V8 is generally successful at flushing the i-cache when needed.

Maybe V8's i-cache flushing sequence for some reason isn't working correctly on the SM-G930F's Exynos8 CPU.
Maybe that CPU reports its cache line sizes (either incorrectly or) in a format that V8 misinterprets. (This guess is derived from the similarity to issue 593867 where cache line size detection was the cause.)
Neither of those two theories would explain the regression range though.
The difference I noticed in the crash on 50.0.2661.26 and 50.0.2661.32 was that, on the .26 build I would see an 'Aw, snap' as soon as I try to navigate to an URL. Where as on the .32 build, the URL tries to load (atleast some content on the page loads and then I see the 'Aw, snap' after couple seconds.
Re: removing RB label, I'd generally agree, but this impacts a flagship device for the most popular Android manufacturer out there - so we need to fix it ASAP.  50.0.2661.35 has been pushed to dev and contains the first fix, so please keep an eye on crash statistics today.  If we see any more occurrences of this crash, please order a device (go out and buy one from a local shop preferably, otherwise order) so that we don't have to cycle debug / fix rounds on a 24h cycle.

Comment 9 by mlliu@chromium.org, Mar 16 2016

I was able to bisect. This CL causes the crash. https://chromium.googlesource.com/v8/v8/+/c3ff68b6b76ff1ffa1f6cd7c82181366020a0fe2
Updates:

(1) Revert is in flight: https://codereview.chromium.org/1806853002/ . Please test with that as soon as you get a chance.

(2) We're expecting to get an SM-G930F in the MUC office tomorrow.

(3) At first I didn't like the idea of reverting c3ff68b6b76ff1ffa1f6cd7c82181366020a0fe2 because as far as I could see that CL just moved code around, and I couldn't see a reason why that would change behavior (so I was skeptical that a revert would help). However yangguo@ provided a possible explanation: if the different cores in a big.LITTLE architecture report different cache line sizes, then that CL will indeed do the wrong thing. That makes sense, so now I'm hopeful that the revert actually will solve the problem.

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/5d62db7430d48e112ddda7178419dc63adb32620

commit 5d62db7430d48e112ddda7178419dc63adb32620
Author: jkummerow <jkummerow@chromium.org>
Date: Wed Mar 16 13:49:39 2016

Revert "Detect cache line size on Linux for PPC hosts."

along with "[arm64] Fix i/d cache line size confusion typo"
and "Fix a warning about inline asm source/destination mismatches..."
which were building on it.

This reverts the following commits:
8d7399f9f8688a93137ced9e084bccb8cc1b075c
474e6a3d6d3482b97c8307e45bb314f6e50bfc3c
c3ff68b6b76ff1ffa1f6cd7c82181366020a0fe2

Reason for revert: We're getting a large number of crash reports from
arm64 devices that are obviously related to cache flushing after code
patching. Bisection results say that the problems started at revision
c3ff68b. Since I can't find a bug in that CL except for the typo that
I've fixed in 474e6a3 (which made some of the crashes go away but not
all of them), we have no choice but to revert the changes in order to
get stability under control while we investigate.

BUG= chromium:594646 
LOG=n

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

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

[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/arm/assembler-arm.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/arm/codegen-arm.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/arm64/assembler-arm64.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/arm64/cpu-arm64.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/assembler.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/assembler.h
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/base/cpu.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/base/cpu.h
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/ppc/assembler-ppc.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/ppc/cpu-ppc.cc
[modify] https://crrev.com/5d62db7430d48e112ddda7178419dc63adb32620/src/ppc/macro-assembler-ppc.cc

Comment 12 by habl...@google.com, Mar 16 2016

Labels: Merge-TBD-5.0
Cc: amineer@chromium.org aska...@chromium.org
Friendly ping, please verify that the revert fixes the problem (for now).

Comment 14 by boliu@chromium.org, Mar 17 2016

 Issue 595745  has been merged into this issue.

Comment 15 by boliu@chromium.org, Mar 17 2016

Cc: mlliu@chromium.org
+mlliu for manual verification

Comment 16 by boliu@chromium.org, Mar 17 2016

Components: Mobile>WebView
Labels: -Restrict-View-Google ReleaseBlock-Stable M-50
Shipped M50 beta webview with this crash. Nothing private here anymore I think so -RVG
Verified fix on 51.0.2681.0 .
I don't see any crash on browsing
Labels: -Merge-TBD-5.0 Merge-Approved-5.0
Ok, let's merge to 5.0.

Comment 19 by aluo@chromium.org, Mar 17 2016

Verified fix on 51.0.2681.0 using Galaxy S7 SM=G930FD MMB29K for WebView.

Comment 20 by kmans...@gmail.com, Mar 21 2016

Hello,

I'm seeing (what looks like) same crash with 50.0.2661.35 "beta", S7 Edge:

https://bugs.chromium.org/p/chromium/issues/detail?id=595745

Seeing that this was verified in 51.0.2681.0 (comments #17, #19):

Will the fix also be merged into the next "beta" 50.* WebView and then the "stable" 50.* WebView?

Project Member

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

Labels: merge-merged-5.0
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f396d7d65fc540daedef7d08632c0c3d7629eab2

commit f396d7d65fc540daedef7d08632c0c3d7629eab2
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Mon Mar 21 16:07:00 2016

Version 5.0.71.21: Merge arm64 CacheLineSizes revert

This reverts:
d627337e9 - 5.0.71.16 / Fix i/d cache line size confusion typo
c3ff68b6b - Detect cache line size on Linux for PPC hosts.
Which is the branch's equivalent of backmerging:
5d62db743 - Revert "Detect cache line size on Linux for PPC hosts."

And cherry-picks:
042f09a95 - Reland PPC portion of "Detect cache line size on Linux for PPC hosts."

BUG= chromium:594646 ,chromium:593867
LOG=n
R=cbruni@chromium.org

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

Cr-Commit-Position: refs/branch-heads/5.0@{#28}
Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1}
Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215}

[modify] https://crrev.com/f396d7d65fc540daedef7d08632c0c3d7629eab2/include/v8-version.h
[modify] https://crrev.com/f396d7d65fc540daedef7d08632c0c3d7629eab2/src/arm64/assembler-arm64.cc
[modify] https://crrev.com/f396d7d65fc540daedef7d08632c0c3d7629eab2/src/arm64/cpu-arm64.cc
[modify] https://crrev.com/f396d7d65fc540daedef7d08632c0c3d7629eab2/src/base/cpu.cc

Labels: -Merge-Approved-5.0
Status: Fixed (was: Assigned)
Fixed and merged, we're all done here.

#20: yes, see #18/#21. The next builds that will be cut will contain the fix.
Verified the issue not reproducible in the latest M 50 build.

Sign in to add a comment