Geo URI crash |
||||||||||||||||
Issue descriptionUserAgent: 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
,
Oct 9
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?
,
Oct 9
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.
,
Oct 9
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.
,
Oct 9
I could repro after navigating to example.com and entering the following in DevTools console: location = "geo:37.786971,-122.399677"
,
Oct 9
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(...) ...
,
Oct 9
Things in #c6 look ARC-related ("intent", "arc::" namespace) - changing the components of the bug based on this.
,
Oct 9
,
Oct 9
,
Oct 9
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?
,
Oct 9
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.
,
Oct 12
I will follow up on monday, sorry about the delay o/
,
Oct 15
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.
,
Oct 15
CL at https://chromium-review.googlesource.com/c/chromium/src/+/1280090 (Moday is earlier in AU than every else!)
,
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
,
Oct 16
Should be fixed by #15. I will try and verify on Canary before requesting a merge.
,
Oct 17
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)
,
Oct 17
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
,
Oct 17
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.
,
Oct 18
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
,
Oct 18
Rejecting on M70 as we're really close to Stable. Let's monitor this through M71 to make sure things look good. Thanks.
,
Oct 19
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
,
Oct 19
,
Oct 23
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 |
||||||||||||||||
Comment 1 by devdeepray@chromium.org
, Oct 8