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

Issue 893042 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Geo URI crash

Project Member Reported by devdeepray@chromium.org, Oct 8

Issue description

UserAgent: Mozilla/5.0 (X11; CrOS x86_64 10895.56.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.95 Safari/537.36
Platform: 10895.56.0 (Official Build) stable-channel eve

Steps to reproduce the problem:
1. Visit https://en.wikipedia.org/wiki/Geo_URI_scheme
2. Click on any Geo URI
3. Voila

What is the expected behavior?
Does not crash.

What went wrong?
Chrome OS crash

Crashed report ID: 

How much crashed? Chrome OS soft reboot

Is it a problem with a plugin? N/A 

Did this work before? N/A 

Chrome version: 69.0.3497.95  Channel: stable
OS Version: 10895.56.0
Flash Version: 31.0.0.108
 
Components: UI>Browser>Navigation
Labels: Needs-Feedback
Status: Unconfirmed (was: Untriaged)
Hmm, I'm not able to repro on a Chromebook Pixel 2 with 69.0.3497.95.  I get a dialog saying "Google Chrome OS can't open this page" instead.  Are there other steps needed to crash?  Do you see this in a guest login, or on other Chromebooks?  Is there a crash ID for it in chrome://crashes?
Status: Untriaged (was: Unconfirmed)
I got a repro on a Pixelbook 69.0.3497.95 by just clicking a link to "geo:37.786971,-122.399677" on the repro page.  Unfortunately, I don't see any crashes for this under chrome://crashes.
I also could repro on HP Chromebook 13 with 69.0.3497.95 (using original repro steps).  Interestingly, I cannot repro when entering "geo:37.786971,-122.399677" into the omnibox.  And I also didn't see any crashes at chrome://crashes.
I could repro after navigating to example.com and entering the following in DevTools console:
    location = "geo:37.786971,-122.399677"
I think that the crashes got uploaded after I launched crosh (ctrl-shift-t) and run |upload_crashes| - I can now see the following report ids with timestamps that look reasonable:
3fd36efc9d66113f
457028e6ba4651a6
0853637d8cc84538

Looking at the middle crash report, it seems that the crash happens here:
https://chromium.googlesource.com/chromium/src.git/ui/views/view.h@f33de8faba398a4a439492f761b5589b05ff2da9 :

296	tfarina	7 years	  const View* child_at(int index) const {
297	tfarina	7 years	    DCHECK_GE(index, 0);
298	ben	7 years	    DCHECK_LT(index, child_count());
299	ben	7 years	    return children_[index];
300	ben	7 years	  }

Callstack:

	ui/views/view.h:299	IntentPickerBubbleView::ShowBubble(...)
	chrome/browser/ui/views/toolbar/toolbar_view.cc:298	ToolbarView::ShowIntentPickerBubble(...)
	chrome/browser/ui/views/frame/browser_view.cc:1214	BrowserView::ShowIntentPickerBubble(...)
	chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:471	arc::(anonymous namespace)::OnAppIconsReceived(...)
	...
Components: -UI>Browser>Navigation Platform>Apps>ARC
Things in #c6 look ARC-related ("intent", "arc::" namespace) - changing the components of the bug based on this.
Components: Internals>Views
Labels: -Needs-Feedback
Cc: dominickn@chromium.org
Owner: djacobo@chromium.org
Ah, I have ARC++ disabled on my Chromebook, which is probably why the crash didn't repro for me in comment 2.

djacobo@ and dominickn@, it looks like you might be familiar with the intent helper code.  Could one of you take a look?
Labels: M-71
I'm away from my desk (and hence my Pixelbook) until next Monday. If David hasn't taken a look by then, I'll investigate - I didn't see an obvious cause for this so I'll need to push a debug build to my Pixelbook and trigger the bug to see exactly what's going wrong.

We should target a fix for M71 in any case, since crashing is bad.
I will follow up on monday, sorry about the delay o/
Cc: -dominickn@chromium.org djacobo@chromium.org
Owner: dominickn@chromium.org
Status: Started (was: Untriaged)
The problem is https://cs.chromium.org/chromium/src/chrome/browser/ui/views/intent_picker_bubble_view.cc?q=markasselected&sq=package:chromium&l=168

If Chrome is the only candidate provided (e.g. for Geo URI links), IntentPickerBubbleView::Init() erases it from the list of candidates, and then we try and mark the first candidate as selected, which crashes due to index out of bounds. The fix is easy and we should merge it. 
CL at https://chromium-review.googlesource.com/c/chromium/src/+/1280090 (Moday is earlier in AU than every else!)
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 15

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

commit 3929b82ef77743c8f0e5d775b2b04eb5f3de95f0
Author: Dominick Ng <dominickn@chromium.org>
Date: Mon Oct 15 23:21:46 2018

Ensure the intent picker is not created when Chrome is the only app candidate.

The IntentPickerBubbleView is provided with a list of app candidates to
display to the user for them to choose to open a link. If Chrome can
handle a link, it is included in the list of candidates, but it is
erased in IntentPickerBubbleView::Init(); users use an independent
button in the UI to open the link in Chrome.

This led to a browser crash bug in the ArcExternalProtocolHandler,
where it was possible for Chrome to be the only candidate available,
e.g. in the case of a geo:// URI scheme link. On clicking a geo:// link
on a Chrome OS devices with Play enabled and no Android apps available
to handle the geo:// URI scheme, the intent picker would try and
select the 0th index app candidate during its initialization, but this
would be null since the only candidate (Chrome) was erased.

This CL fixes the bug by not creating the intent picker at all if the
only app candidate is Chrome. The external protocol dialog can only be
triggered if Chrome originally decided it couldn't handle the URL, so in
this case we simply fall back to the Chrome OS standard dialog (which
says that Chrome OS can't handle the URL).

BUG= 893042 

Change-Id: I171d547e51a4dfb37d51c3e71a88a8c84539a8fa
Reviewed-on: https://chromium-review.googlesource.com/c/1280090
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599773}
[modify] https://crrev.com/3929b82ef77743c8f0e5d775b2b04eb5f3de95f0/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/3929b82ef77743c8f0e5d775b2b04eb5f3de95f0/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/3929b82ef77743c8f0e5d775b2b04eb5f3de95f0/chrome/browser/ui/views/intent_picker_bubble_view.h

Should be fixed by #15. I will try and verify on Canary before requesting a merge.
Labels: Merge-Request-70 Merge-Request-71
I just tested Cros 11167.0.0, it has Dominick's fix and its WAI, it shows

"Google Chrome OS can't open this page." when there are no installed app candidates that can handle the geo: scheme.

Requesting Merge for M71 (also M70 if possible)
Project Member

Comment 18 by sheriffbot@chromium.org, Oct 17

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This crash has been around for a long time, so it's understandable if the merge to M70 is rejected. However, this change only affects Chrome OS, not any other platform, so technically M70 isn't out yet there. We should definitely merge to M71 though.
Project Member

Comment 20 by sheriffbot@chromium.org, Oct 18

Labels: -Merge-Request-71 Hotlist-Merge-Approved Merge-Approved-71
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-70 Merge-Rejected-70
Rejecting on M70 as we're really close to Stable. Let's monitor this through M71 to make sure things look good. Thanks.
Project Member

Comment 22 by bugdroid1@chromium.org, Oct 19

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0737d669700537a0df4b455175f9b2815277e5b8

commit 0737d669700537a0df4b455175f9b2815277e5b8
Author: Dominick Ng <dominickn@chromium.org>
Date: Fri Oct 19 00:02:17 2018

Ensure the intent picker is not created when Chrome is the only app candidate.

The IntentPickerBubbleView is provided with a list of app candidates to
display to the user for them to choose to open a link. If Chrome can
handle a link, it is included in the list of candidates, but it is
erased in IntentPickerBubbleView::Init(); users use an independent
button in the UI to open the link in Chrome.

This led to a browser crash bug in the ArcExternalProtocolHandler,
where it was possible for Chrome to be the only candidate available,
e.g. in the case of a geo:// URI scheme link. On clicking a geo:// link
on a Chrome OS devices with Play enabled and no Android apps available
to handle the geo:// URI scheme, the intent picker would try and
select the 0th index app candidate during its initialization, but this
would be null since the only candidate (Chrome) was erased.

This CL fixes the bug by not creating the intent picker at all if the
only app candidate is Chrome. The external protocol dialog can only be
triggered if Chrome originally decided it couldn't handle the URL, so in
this case we simply fall back to the Chrome OS standard dialog (which
says that Chrome OS can't handle the URL).

BUG= 893042 

Change-Id: I171d547e51a4dfb37d51c3e71a88a8c84539a8fa
Reviewed-on: https://chromium-review.googlesource.com/c/1280090
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599773}(cherry picked from commit 3929b82ef77743c8f0e5d775b2b04eb5f3de95f0)
Reviewed-on: https://chromium-review.googlesource.com/c/1290249
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#140}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/0737d669700537a0df4b455175f9b2815277e5b8/chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
[modify] https://crrev.com/0737d669700537a0df4b455175f9b2815277e5b8/chrome/browser/ui/views/intent_picker_bubble_view.cc
[modify] https://crrev.com/0737d669700537a0df4b455175f9b2815277e5b8/chrome/browser/ui/views/intent_picker_bubble_view.h

Status: Fixed (was: Started)
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/0737d669700537a0df4b455175f9b2815277e5b8

Commit: 0737d669700537a0df4b455175f9b2815277e5b8
Author: dominickn@chromium.org
Commiter: dominickn@chromium.org
Date: 2018-10-19 00:02:17 +0000 UTC

Ensure the intent picker is not created when Chrome is the only app candidate.

The IntentPickerBubbleView is provided with a list of app candidates to
display to the user for them to choose to open a link. If Chrome can
handle a link, it is included in the list of candidates, but it is
erased in IntentPickerBubbleView::Init(); users use an independent
button in the UI to open the link in Chrome.

This led to a browser crash bug in the ArcExternalProtocolHandler,
where it was possible for Chrome to be the only candidate available,
e.g. in the case of a geo:// URI scheme link. On clicking a geo:// link
on a Chrome OS devices with Play enabled and no Android apps available
to handle the geo:// URI scheme, the intent picker would try and
select the 0th index app candidate during its initialization, but this
would be null since the only candidate (Chrome) was erased.

This CL fixes the bug by not creating the intent picker at all if the
only app candidate is Chrome. The external protocol dialog can only be
triggered if Chrome originally decided it couldn't handle the URL, so in
this case we simply fall back to the Chrome OS standard dialog (which
says that Chrome OS can't handle the URL).

BUG= 893042 

Change-Id: I171d547e51a4dfb37d51c3e71a88a8c84539a8fa
Reviewed-on: https://chromium-review.googlesource.com/c/1280090
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599773}(cherry picked from commit 3929b82ef77743c8f0e5d775b2b04eb5f3de95f0)
Reviewed-on: https://chromium-review.googlesource.com/c/1290249
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#140}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Sign in to add a comment