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

Issue 859386 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Clank sad tab/aw snap

Project Member Reported by mikelawther@chromium.org, Jul 2

Issue description

Chrome Version: 69.0.3475.0
OS: Android 8.1.0 (on Pixel 2 XL)

What steps will reproduce the problem?
(1) Visit http://jalopnik.com
(2) Click any story
(3) Press back arrow (the Android system one)

What is the expected result?

It goes back to the jalopnik.com main page

What happens instead?

I get a sad tab. If I click 'reload' on the sad tab screen, the main page reloads OK.

I can't repro on other Chrome versions on my phone (eg stable), so this may somehow be specific to me or my profile? It's 100% reproducible (across phone reboots and OS upgrades) on the version is does happen on though.

Looking in about:crashes, I don't see any crash recorded for this. This has been happening for a while now.

 
Labels: Needs-triage-Mobile
Cc: jbanavatu@chromium.org
Components: UI>Browser>Navigation
Labels: -Type-Bug -Pri-3 ReleaseBlock-Dev M-69 RegressedIn-69 Triaged-Mobile Target-69 FoundIn-69 Pri-1 Type-Bug-Regression
Owner: clamy@chromium.org
Status: Assigned (was: Untriaged)
Tested the issue in Android and able to reproduce the issue. 

Steps Followed:
1. Launch Chrome.
2. Visit http://jalopnik.com
3. Click any story
4. Press back arrow (the Android system one) and Observed tab crash


Chrome versions tested:
69.0.3475.0(Dev), 69.0.3479.2(Latest canary)

OS:
Android 8.1.0

Android Devices:
Pixel 2 XL

Using the per-revision bisect providing the bisect results,
Good Build - 69.0.3447.0 563479
Bad Build - 69.0.3448.0  563958

You are looking for a change made after 563609(GOOD), but before 563610(BAD).

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+/6b92fcfb1cf296130f6cb38213aa52a0324c9e46

@ clamy :  Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to owner concerned.

Please navigate to below link for log's  --
go/chrome-androidlogs/859386

Thanks!
Thanks for bisecting this. 

Any idea why this was not showing up in chrome:crashes? I'm worried that this means this (or other crashes) could be widespread in the wild but we are getting no crash info about it.
Happens in chromeOS too.
Version: Chrome/69.0.3473.0
Platform: 10820.0.0 (Official Build) dev-channel gandof
Firmware: Google_Gandof.6301.155.9

Labels: OS-Chrome
Summary: Clank sad tab/aw snap (was: Clank sad tab/aw snap, but no crash recorded in about:crashes)
Adding ChromeOS per comment #4.

This looks to be two separate problems - the crash itself, but also that it doesn't show up in about:crashes. 

I'll leave this bug to be specific to the crash, and file a new one about it not appearing in about:crashes.
Filed bug #861607 to track the 'not showing in chrome:crashes' problem.
I can't repro on trunk using chrome_public_apk, nor on the latest Canary (69.0.3487.0). If you're still seeing the issue, could you provide a crash stack? Thanks!
Status: WontFix (was: Assigned)
Going to close this out for now; WontFix, cannot reproduce. If the issue continues, please reopen. Thanks!
Status: Assigned (was: WontFix)
Wait a sec, this was reproed by at least three people (me, QA, and an external contributor), across two OSs. And we have a bisect down the suspect CL. Please don't WontFix one hour after a request by the engineer for more information.

@clamy - this is not uploading a crash stack on my phone. 
@jbanavatu - can you help with crash stack information? I can't read the logs link you posted earlier.
@mikelawther: can you still reproduce the issue on the latest Canary? The suspected CL is not easily revertible, and there's been a lot of work going on in the area it touches so the issue might be gone.
FWIW I saw this bug using canary myself a couple weeks back, but it went away for me there as well shortly afterwards, so I'm working under the assumption the issue is fixed.  I also confirmed that I can repro 100% in dev, but not in latest canary using same steps.

So, I'd tend to concur with clamy@ re: this likely having been addressed via another patch.  We could potentially do a reverse bisect to find which CL fixed the issue if we wanted explicit confirmation of which patch fixed this, but I'm unsure if it's worth the time personally, kind of leave that to clamy@ since they'll know best what work has been ongoing in this area.
Thanks for the additional details, Alex. I recommend we remove RBD at this time and proceed with releasing Dev. I recommend updating to RBS for now so we do not lose track of it. If it continues after releasing a new version to dev, we can update to RBD and hope to increase urgency.
Labels: -ReleaseBlock-Dev ReleaseBlock-Beta
Cc: roc...@chromium.org boliu@chromium.org
With DCHECKs enabled, it hit this DCHECK in generated mojo code:
    DCHECK(!connected)
        << "FrameNavigationControl::CommitNavigationCallback was destroyed without "
        << "first either being run or its corresponding binding being closed. "
        << "It is an error to drop response callbacks which still correspond "
        << "to an open interface pipe.";

https://cs.chromium.org/chromium/src/out/Debug/gen/content/common/frame.mojom.cc?rcl=eb2e049e1b98cf00c75d2540b3fb4c32b4d5e6d6&l=250

+rockot for mojo help. So what happens without DCHECKs?

Also I could only reproduce this with site isolation disabled, although not sure if that's important
stack.txt
43.4 KB View Download
rockot: does that just generate a mojo channel error on the browser side if the DCHECk doesn't fire? that would explain the other behavior of not generating a crash report from this, because the renderer process never actually crashed
This means that something is never getting to get a reply to its call to FrameNavigationControl.CommitNavigation[1]. In practice there are quite often no harmful side effects to this because the code responsible for responding will imminently close the binding anyway.

The fix is typically just to make sure that in such cases, the binding is closed before uncalled response callbacks are discarded.
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 13

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

commit 138153b186554c2077e867efbeb71e0342b70159
Author: Ken Rockot <rockot@chromium.org>
Date: Fri Jul 13 23:31:57 2018

IPC: Avoid closing Channel endpoints on error

We never want to explicitly close a Channel endpoint until after the
corresponding child process is dead. This prevents that from happening
in some cases where it's still possible today.

TBR=dcheng@chromium.org

Bug:  859386 ,861607, 863430 
Change-Id: I2858b6e011e06a000ae976c1e6ae96595cf67a85
Reviewed-on: https://chromium-review.googlesource.com/1135712
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575115}
[modify] https://crrev.com/138153b186554c2077e867efbeb71e0342b70159/ipc/DEPS
[modify] https://crrev.com/138153b186554c2077e867efbeb71e0342b70159/ipc/ipc_channel_mojo_unittest.cc
[modify] https://crrev.com/138153b186554c2077e867efbeb71e0342b70159/ipc/ipc_mojo_bootstrap.cc
[modify] https://crrev.com/138153b186554c2077e867efbeb71e0342b70159/ipc/ipc_test.mojom

@boliu, @rockot - THANK YOU for persisting and investigating this! Looks like a genuine bug was fixed here.
Verified fix in 69.0.3497.2 / Pixel 2 XL / 8.1.0. - No Aw,snap on 'back'

Is there anymore work pending here, or the issue can be closed?
Status: Fixed (was: Assigned)
I don't think there's anything pending, closing this issue.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
Labels: -Merge-TBD
Fix was verified in 69.0.3497.2 so no need for merge.
Just to follow up - this issue may have been responsible for ~25% of sad tabs (where the user clicked reload on a sad tab) on Android dev channel (https://uma.googleplex.com/p/chrome/timeline_v2/?sid=f0e14b372b4723cb10fd4ff3b0225875)

In the release after the bisected CL above landed, sad-tab-reload events on Android spiked by ~33%. And then after the fix CL landed, they dropped again.

Sign in to add a comment