1011024 - Fix nsScriptSecurityManager::GetChannelPrincipal so that it doesn't fail to get the correct nsIPrincipal for some resource documents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Hence I left the nsIDocShell code in there.

Are you getting a _docshell_ in the sync-xbl-load case?

Comment 4

[Jonathan Watt [:jwatt]]

Oh, no, I am not, sorry for the confusion. I can remove that code.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Jonathan Watt [:jwatt]]

(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.

Comment 8

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/73cd98bb4f40

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/73cd98bb4f40