SecurityOrigin and Origin disagree on which urls are unique |
|||||||
Issue descriptionAndroid uses urls like this to represent email threads: x-thread://1143759988/1548110181238723403 In Blink, GetSecurityOrigin() returns an origin of "x-thread://" and IsUnique() returns false. If you try to pass that Origin over mojo, unserialization fails in origin_struct_traits.h because url::Origin::unique() returns true since it has no valid host. I'm not sure if SecurityOrigin should be setting is_unique to true. I don't quite understand right now why it always sets unique to false if you construct it with a KURL. Alternatively, maybe the bug is just in origin_struct_traits.h - perhaps it should just fail more gracefully by computing unique() itself rather than trusting the value passed through from the renderer.
,
Sep 6 2017
,
Sep 6 2017
Per https://url.spec.whatwg.org/#origin, it seems like SecurityOrigin in blink should be treating this as unique?
,
Sep 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba899c918862bf984e01ed525df23a21ffba4ea1 commit ba899c918862bf984e01ed525df23a21ffba4ea1 Author: Dominic Mazzoni <dmazzoni@chromium.org> Date: Thu Sep 07 21:29:54 2017 Work around bug when sending nonstandard Android origin over mojo. This is a quick fix so that opening a Gmail message on Android doesn't crash when accessibility is enabled, due to http://crbug.com/759528 causing a crash when sending a nonstandard origin over mojo. Once the underlying bug is fixed we can remove this check. Bug: 762716, 759528 Change-Id: I87b249f04aef0b19fd8425b1f89639b488fb0f6d Reviewed-on: https://chromium-review.googlesource.com/653435 Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Alice Boxhall <aboxhall@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#500391} [modify] https://crrev.com/ba899c918862bf984e01ed525df23a21ffba4ea1/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp
,
Sep 9 2017
Fix has been verified in crbug/759528
,
Sep 11 2017
If Chrome doesn't understand the URL, we should be treating it as unique. I don't know what it would mean to take the origin of an `x-thread://` URL... where is that heppaning? Why are we passing it around as an origin?
,
Sep 11 2017
This problem is showing up because I added AXObjectCacheImpl::AddPermissionStatusListener, which requests the status of a particular permission as soon as the document loads. AddPermissionObserver requires the SecurityOrigin. It's not obvious to me that this code is wrong. There are lots of urls, like file urls, that don't have an origin but ought to be fair game for requesting certain permissions. I suspect it'd be possible to trigger this same crash by loading an unusual url (in an Android WebView for example) that has some JavaScript to query some other permission, like geolocation. The only reason this crash is showing up is because we're requesting the permission on page load when accessibility is on.
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b78a4260a2a4bd43266828a7cd30c58e98b67a4b commit b78a4260a2a4bd43266828a7cd30c58e98b67a4b Author: Dominic Mazzoni <dmazzoni@chromium.org> Date: Mon Sep 11 16:41:08 2017 Merge to M62: Work around bug when sending nonstandard Android origin over mojo. This is a quick fix so that opening a Gmail message on Android doesn't crash when accessibility is enabled, due to http://crbug.com/759528 causing a crash when sending a nonstandard origin over mojo. Once the underlying bug is fixed we can remove this check. Bug: 762716, 759528 Change-Id: I87b249f04aef0b19fd8425b1f89639b488fb0f6d Reviewed-on: https://chromium-review.googlesource.com/653435 Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Alice Boxhall <aboxhall@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#500391}(cherry picked from commit ba899c918862bf984e01ed525df23a21ffba4ea1) Reviewed-on: https://chromium-review.googlesource.com/660601 Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#127} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/b78a4260a2a4bd43266828a7cd30c58e98b67a4b/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp
,
Sep 13 2017
I'm going to remove the beta blocking tag since a workaround has landed on trunk / branch but leave this bug open to track the fix for realz. Holler or re-update the bug if this is incorrect. Note we plan to ship M62 to beta this afternoon so holler quickly :P
,
Nov 10 2017
,
Feb 18 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by dmazz...@chromium.org
, Sep 6 2017Labels: OS-Android