Issue metadata
Sign in to add a comment
|
Add DCHECK which ensures that valid origins are passed to GetPermissionStatus |
||||||||||||||||||||||
Issue descriptionThings can fail in unexpected ways if empty/invalid GURLs are passed. Initial attempt here: https://chromium-review.googlesource.com/c/chromium/src/+/576409
,
Nov 6 2017
Feel free to take a look. The main thing that has to be fixed are failing tests.
,
Nov 6 2017
,
Nov 7 2017
Dear Raymes, I debugged few failues as below: Callstack : Because of our decheck [24891:24891:1106/220952.965221:FATAL:permission_context_base.cc(229)] Check failed: requesting_origin.is_valid() && embedding_origin.is_valid(). #0 0x000003b7f6bc base::debug::StackTrace::StackTrace() #1 0x000003b9959c logging::LogMessage::~LogMessage() #2 0x000003d68733 PermissionContextBase::GetPermissionStatus() #3 0x000003d6d226 PermissionManager::GetPermissionStatusHelper() #4 0x000003d6d0f9 PermissionManager::GetPermissionStatus() #5 0x000006a988be PageInfo::PresentSitePermissions() #6 0x000006a9653a PageInfo::PageInfo() #7 0x0000069a6616 PageInfoBubbleView::PageInfoBubbleView() #8 0x0000069a5f77 PageInfoBubbleView::CreatePageInfoBubble() #9 0x0000069a7c22 ShowPageInfoDialogImpl() #10 0x000006877393 ShowPageInfoDialog() #11 0x000001227b98 (anonymous namespace)::OpenPageInfoBubble() #12 0x000001227a22 PageInfoBubbleViewBrowserTest_ShowBubble_Test::RunTestOnMainThread() LOGS as below [31778:31778:1107/183549.833640:ERROR:page_info_dialog.cc(33)] CHANDRA :: ShowPageInfoDialog URL = about:blank [31778:31778:1107/183549.833693:ERROR:page_info_bubble_view.cc(463)] CHANDRA :: PageInfoBubbleView::CreatePageInfoBubble URL =about:blank [31778:31778:1107/183549.834829:ERROR:page_info_bubble_view.cc(556)] CHANDRA :: PageInfoBubbleView::PageInfoBubbleView URL = about:blank [31778:31778:1107/183549.834919:ERROR:page_info.cc(823)] CHANDRA :: PageInfo::PageInfo URL=about:blank [31778:31778:1107/183549.834948:ERROR:permission_manager.cc(603)] CHANDRA :: PermissionManager::GetPermissionStatusHelper requesting_origin = about:blank [31778:31778:1107/183549.834968:ERROR:permission_manager.cc(604)] CHANDRA :: PermissionManager::GetPermissionStatusHelper embedding_origin = about:blank [31778:31778:1107/183549.835001:ERROR:permission_manager.cc(608)] CHANDRA :: PermissionManager::GetPermissionStatusHelper canonical_requesting_origin = about:blank [31778:31778:1107/183549.835020:ERROR:permission_manager.cc(609)] CHANDRA :: PermissionManager::GetPermissionStatusHelper canonical_requesting_origin.GetOrigin() = [31778:31778:1107/183549.835042:ERROR:permission_manager.cc(610)] CHANDRA :: PermissionManager::GetPermissionStatusHelper embedding_origin.GetOrigin() = [31778:31778:1107/183549.835065:ERROR:permission_context_base.cc(229)] CHANDRA :: PermissionContextBase::GetPermissionStatus requesting_origin = [31778:31778:1107/183549.835085:ERROR:permission_context_base.cc(232)] CHANDRA :: PermissionContextBase::GetPermissionStatus embedding_origin = Note: As per my understanding GetOrigin() returning EMPTY as its not standard url. Plz suggest
,
Nov 7 2017
Majority failures due to same reason. Do we need to modify test cases with some valid urls or modify chromium code? Plz suggest. Thanks in advance
,
Nov 10 2017
@Raymes, Could you please check and let me know
,
Nov 10 2017
,
Nov 10 2017
Sorry, I'm really busy with some urgent things but I'll try to take a look at this late next week.
,
Nov 15 2017
cm.sanchi: interesting. From a quick play page info is available on about:blank, so removing those DCHECKs, going to about:blank, and bringing up page info will probably cause the DCHECK to fail. Is that right?
,
Nov 15 2017
sorry that should say, '... so adding those DCHECKs, going to ....'
,
Nov 15 2017
I mean to say below function returns NULL as its not a standard URL.
GURL GURL::GetOrigin() const {
// This doesn't make sense for invalid or nonstandard URLs, so return
// the empty URL.
if (!is_valid_ || !IsStandard())
return GURL();
if (SchemeIsFileSystem())
return inner_url_->GetOrigin();
url::Replacements<char> replacements;
replacements.ClearUsername();
replacements.ClearPassword();
replacements.ClearPath();
replacements.ClearQuery();
replacements.ClearRef();
return ReplaceComponents(replacements);
}
So, adding DECHECK fails because we do validate for corrrect urls
DCHECK(requesting_origin.is_valid() && embedding_origin.is_valid());
,
Nov 20 2017
Right. Yurch. What happens is, permission manager calls .GetOrigin() on urls before passing them into permission context base. GetOrigin does something weird with about:blank, and returns an empty URL. I think there are two approaches here: 1. Convert everything to use proper url::Origins, instead of GURLs for origins. This way weird things like about:blank can just be represented as unique origins and basically ignored by the permissions system. 2. Specially handle about:blank in ... various places. Maybe in GetOrigin ... maybe in PermissionManager and places that call PermissionContextBase ... maybe the UI. I prefer 1. but I think we have to figure out file: URLs before we can do that.
,
Nov 20 2017
+Dom as I mentioned file URLs in #12 and he was talking about them the other day.
,
Feb 18 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by cm.san...@samsung.com
, Nov 6 2017