Closed
Bug 1011024
Opened 10 years ago
Closed 10 years ago
Fix nsScriptSecurityManager::GetChannelPrincipal so that it doesn't fail to get the correct nsIPrincipal for some resource documents
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jwatt, Assigned: jwatt)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
If I add an nsIDocument::AllowXULXBL() call to nsDocumentViewer::CreateStyleSet I get a debug abort in nsPrincipal::GetAppId (because the mAppId is UNKNOWN_APP_ID) when running reftests/svg/use-01-extref.svg: nsPrincipal::GetAppId nsPermissionManager::CommonTestPermission nsPermissionManager::TestPermissionFromPrincipal TestSitePerm nsContentUtils::IsSitePermAllow nsContentUtils::AllowXULXBLForPrincipal nsDocument::InternalAllowXULXBL nsIDocument::AllowXULXBL nsIDocument::LoadsFullXULStyleSheetUpFront nsDocumentViewer::CreateStyleSet nsDocumentViewer::InitPresentationStuff The test loads use-01-extref-resource.svg as a resource document, which in turn loads the test's own file (use-01-extref.svg) as a nested resource document. So we end up with: file:///.../layout/reftests/svg/use-01-extref.svg file:///.../layout/reftests/svg/use-01-extref-resource.svg file:///.../layout/reftests/svg/use-01-extref.svg The nested instance of use-01-extref.svg has its document nsIPrincipal set under nsDocument::Reset after that function calls nsScriptSecurityManager::GetChannelPrincipal to get the nsIPrincipal from the nsIChannel (nsFileChannel) that is passed in. nsScriptSecurityManager::GetChannelPrincipal nsDocument::Reset mozilla::dom::XMLDocument::Reset nsDocument::StartDocumentLoad mozilla::dom::XMLDocument::StartDocumentLoad nsContentDLF::CreateDocument nsContentDLF::CreateInstance nsExternalResourceMap::PendingLoad::SetupViewer nsExternalResourceMap::PendingLoad::OnStartRequest nsBaseChannel::OnStartRequest nsInputStreamPump::OnStateStart nsInputStreamPump::OnInputStreamReady nsInputStreamReadyEvent::Run Turns out that channel doesn't have an owner, or an nsIDocShell, though, so GetChannelPrincipal uses UNKNOWN_APP_ID the principal that it returns, which is what causes us to abort later in nsPrincipal::GetAppId(). I'm not sure what the expected behavior is here, but I'm guessing that the entries created for an nsExternalResourceMap should be using an nsFileChannel (nsIRequest/nsIChannel) which has its owner set to the embedding document, or maybe even the mDisplayDocument ancestor, if that's a different document.
Assignee | ||
Updated•10 years ago
|
Summary: Figure out why resource documents may not have a useful nsIPrinciple → Figure out why resource documents may not have a useful nsIPrincipal
Comment 1•10 years ago
|
||
The problem is that nsScriptSecurityManager::GetChannelPrincipal is broken. It's trying to query a docshell from notification callbacks, which is explicitly NOT guaranteed. What's guaranteed is an nsILoadContext. Jonathan, are you up for trying to fix this stuff?
Flags: needinfo?(jwatt)
Assignee | ||
Comment 2•10 years ago
|
||
Like this? Note that we don't seem to always get a nsILoadContext. Specifically I noticed that we failed to get one when loading chrome://global/skin/toolbar.css and chrome://global/skin/menu.css with the following stack: nsScriptSecurityManager::GetChannelPrincipal mozilla::css::SheetLoadData::OnStreamComplete nsUnicharStreamLoader::OnStopRequest nsSyncLoadService::PushSyncStreamToListener mozilla::css::Loader::LoadSheet mozilla::css::Loader::InternalLoadNonDocumentSheet mozilla::css::Loader::LoadSheetSync mozilla::css::Loader::LoadSheetSync nsXBLResourceLoader::LoadResources nsXBLPrototypeResources::LoadResources nsXBLPrototypeBinding::LoadResources nsXBLService::GetBinding nsXBLService::GetBinding nsXBLService::GetBinding nsXBLService::LoadBindings nsCSSFrameConstructor::AddFrameConstructionItemsInternal nsCSSFrameConstructor::AddFrameConstructionItems nsCSSFrameConstructor::ProcessChildren nsCSSFrameConstructor::ConstructDocElementFrame nsCSSFrameConstructor::ContentRangeInserted nsCSSFrameConstructor::ContentInserted PresShell::Initialize mozilla::dom::XULDocument::StartLayout mozilla::dom::XULDocument::DoneWalking mozilla::dom::XULDocument::ResumeWalk mozilla::dom::XULDocument::OnScriptCompileComplete NotifyOffThreadScriptCompletedRunnable::Run nsThread::ProcessNextEvent Hence I left the nsIDocShell code in there.
Attachment #8423941 -
Flags: review?(bzbarsky)
Flags: needinfo?(jwatt)
Comment 3•10 years ago
|
||
> Hence I left the nsIDocShell code in there.
Are you getting a _docshell_ in the sync-xbl-load case?
Assignee | ||
Comment 4•10 years ago
|
||
Oh, no, I am not, sorry for the confusion. I can remove that code.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8423941 -
Attachment is obsolete: true
Attachment #8423941 -
Flags: review?(bzbarsky)
Attachment #8423967 -
Flags: review?(bzbarsky)
Comment 6•10 years ago
|
||
Comment on attachment 8423967 [details] [diff] [review] patch r=me, but: 1) Please mark those two properties as [infallible] in the IDL so you don't have to mess with the out params. 2) nsDocument::ResetToURI should use the new thing, since we have that now.
Attachment #8423967 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #6) > 1) Please mark those two properties as [infallible] in the IDL so you don't > have to mess with the out params. As noted on IRC we can only do that for [builtinclass], which this is not.
Summary: Figure out why resource documents may not have a useful nsIPrincipal → Fix nsScriptSecurityManager::GetChannelPrincipal so that it doesn't fail to get the correct nsIPrincipal for some resource documents
Assignee | ||
Comment 8•10 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/73cd98bb4f40
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/73cd98bb4f40
Assignee: nobody → jwatt
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•