Closed
Bug 997987
Opened 11 years ago
Closed 11 years ago
Simplify the path to getting a subject principal
Categories
(Core :: Security: CAPS, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Ms2ger
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
If somebody does nsContentUtils::GetSubjectPrincipal, the minimal path to the actual work is the following: nsContentUtils::GetSubjectPrincipal() nsScriptSecurityManager::GetSubjectPrincipal(nsIPrincipal **) nsScriptSecurityManager::doGetSubjectPrincipal(nsresult *) nsScriptSecurityManager::GetSubjectPrincipal(JSContext *, nsresult *) That's not even including the refcounting, and the branches and additional calls in the case that the ssm decides to return null. I'm going to clean this up and put everything in nsContentUtils, where it's easy to use.
Comment 8•11 years ago
|
||
Comment on attachment 8408759 [details] [diff] [review] Part 3 - Remove usage of nsIScriptSecurityManager::GetSubjectPrincipal. v1 Review of attachment 8408759 [details] [diff] [review]: ----------------------------------------------------------------- This should really be the first patch in the series, before changing nsIScriptSecurityManager::GetSubjectPrincipal's behaviour. We change behaviour in so many places, though, that I don't want to sign off on this, sorry. ::: caps/src/nsScriptSecurityManager.cpp @@ +337,5 @@ > > ///////////////// Security Checks ///////////////// > > bool > nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx) Assertion about the cx being current and/or remove the argument. @@ +466,5 @@ > (aFirst->GetIsInBrowserElement() == aSecond->GetIsInBrowserElement())); > } > > NS_IMETHODIMP > nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI) Assertion about the cx being current and/or remove the argument. @@ +469,5 @@ > NS_IMETHODIMP > nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI) > { > // Get principal of currently executing script. > nsresult rv; Move this where's it's first assigned @@ +1088,5 @@ > > //-- Access denied, report an error > NS_ConvertUTF8toUTF16 strName("CreateWrapperDenied"); > nsAutoCString origin; > nsresult rv2; Move down (and make it 'rv', and remove the confused comment about why it'd be 'rv2'). ::: content/base/src/DOMParser.cpp @@ -459,5 @@ > if (!cx) { > rv.Throw(NS_ERROR_UNEXPECTED); > return; > } > - Leave this ::: content/base/src/nsDocument.cpp @@ -6347,5 @@ > - > - if (!subject) { > - // Fall back to our principal. Or should we fall back to the null > - // principal? The latter would just mean no binding loads.... > - subject = NodePrincipal(); This looks like a behaviour change. ::: content/html/document/src/nsHTMLDocument.cpp @@ +2848,5 @@ > > void > nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv) > { > + if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) { Can this happen? @@ +2849,5 @@ > void > nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv) > { > + if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) { > + rv.Throw(NS_ERROR_DOM_PROP_ACCESS_DENIED); Two-space ::: docshell/base/nsDocShell.cpp @@ -8670,5 @@ > - // the parent before allowing it to load anything into this > - // docshell. > - nsCOMPtr<nsIPrincipal> subjPrincipal; > - rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjPrincipal)); > - NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal, rv); So this would return for system principals before? Should it keep doing that? @@ +8679,5 @@ > } > > + if (nsContentUtils::GetSubjectPrincipal()->Subsumes(p)) { > + // Same origin, permit load > + return NS_OK;; ;; ::: dom/base/nsGlobalWindow.cpp @@ +6039,5 @@ > // Try to get a host from the running principal -- this will do the > // right thing for javascript: and data: documents. > > nsresult rv = NS_OK; > + nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal(); Behaviour change if called without a current context, I think @@ -11690,5 @@ > // Note the direction of this test: We don't allow setTimeouts running with > // chrome privileges on content windows, but we do allow setTimeouts running > // with content privileges on chrome windows (where they can't do very much, > // of course). > - rv = ourPrincipal->Subsumes(subjectPrincipal, &subsumes); What does this do when called with null? ::: dom/src/storage/DOMStorage.cpp @@ -240,5 @@ > return false; > } > > // chrome can always use aStorage regardless of permission preferences > - if (nsContentUtils::IsCallerChrome()) { Why change this? @@ +278,5 @@ > } > } > > if (aStorage) { > + return aStorage->CanAccess(subject); Name change seems pointless ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp @@ +785,5 @@ > > // Now we have to set the right opener principal on the new window. Note > // that we have to do this _before_ starting any URI loads, thanks to the > + // sync nature of javascript: loads. > + nsCOMPtr<nsIPrincipal> subjectPrincipal = nsContentUtils::GetSubjectPrincipal(); There's a null-check below ::: layout/style/nsCSSStyleSheet.cpp @@ -1632,5 @@ > - nsresult rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal)); > - NS_ENSURE_SUCCESS(rv, rv); > - > - if (!subjectPrincipal) { > - return NS_ERROR_DOM_SECURITY_ERR; The number of places where we throw exceptions for null is... interesting.
Attachment #8408759 -
Flags: review?(Ms2ger) → feedback+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to :Ms2ger from comment #8) > This should really be the first patch in the series, before changing > nsIScriptSecurityManager::GetSubjectPrincipal's behaviour. Well, my intention was to do all of the behavior change in one small patch, and then make the big patch a simple mass-conversion. This makes sense from a bisecting perspective, but you're right that it makes it harder to review. So I'm just going to ditch Part 1, and modify Part 3 to preserve the old semantics in all but one place. Hopefully you'll feel more comfortable reviewing this version, since I think Boris is pretty overloaded right now. > ::: caps/src/nsScriptSecurityManager.cpp > > bool > > nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx) > > Assertion about the cx being current and/or remove the argument. Done. > > NS_IMETHODIMP > > nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI) > > Assertion about the cx being current and/or remove the argument. Done. > > nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI) > > { > > // Get principal of currently executing script. > > nsresult rv; > > Move this where's it's first assigned Done. > Move down (and make it 'rv', and remove the confused comment about why it'd > be 'rv2'). Done. > ::: content/base/src/DOMParser.cpp > @@ -459,5 @@ > > if (!cx) { > > rv.Throw(NS_ERROR_UNEXPECTED); > > return; > > } > > - > > Leave this Done. > ::: content/base/src/nsDocument.cpp > @@ -6347,5 @@ > > - > > - if (!subject) { > > - // Fall back to our principal. Or should we fall back to the null > > - // principal? The latter would just mean no binding loads.... > > - subject = NodePrincipal(); > > This looks like a behaviour change. Great catch! I've fixed it to preserve the old semantics. > ::: content/html/document/src/nsHTMLDocument.cpp > @@ +2848,5 @@ > > > > void > > nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv) > > { > > + if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) { > > Can this happen? I don't know, but I don't want to touch it in this patch. > > @@ +2849,5 @@ > > void > > nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv) > > { > > + if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) { > > + rv.Throw(NS_ERROR_DOM_PROP_ACCESS_DENIED); > > Two-space Fixed. > ::: docshell/base/nsDocShell.cpp > @@ -8670,5 @@ > > - // the parent before allowing it to load anything into this > > - // docshell. > > - nsCOMPtr<nsIPrincipal> subjPrincipal; > > - rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjPrincipal)); > > - NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal, rv); > > So this would return for system principals before? Should it keep doing that? I'm pretty sure the old behavior is nonsensical, but I've fixed up the patch to behave the same way. > > + if (nsContentUtils::GetSubjectPrincipal()->Subsumes(p)) { > > + // Same origin, permit load > > + return NS_OK;; > > ;; Fixed. > ::: dom/base/nsGlobalWindow.cpp > @@ +6039,5 @@ > > // Try to get a host from the running principal -- this will do the > > // right thing for javascript: and data: documents. > > > > nsresult rv = NS_OK; > > + nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal(); > > Behaviour change if called without a current context, I think Same as the above. > > @@ -11690,5 @@ > > // Note the direction of this test: We don't allow setTimeouts running with > > // chrome privileges on content windows, but we do allow setTimeouts running > > // with content privileges on chrome windows (where they can't do very much, > > // of course). > > - rv = ourPrincipal->Subsumes(subjectPrincipal, &subsumes); > > What does this do when called with null? If if ourPrincipal is System, it will return true. Otherwise it will return false. Luckily, those are the same semantics we'll get when null -> nsSystemPrincipal. > > ::: dom/src/storage/DOMStorage.cpp > @@ -240,5 @@ > > return false; > > } > > > > // chrome can always use aStorage regardless of permission preferences > > - if (nsContentUtils::IsCallerChrome()) { > > Why change this? To avoid computing the subject principal twice. > @@ +278,5 @@ > > } > > } > > > > if (aStorage) { > > + return aStorage->CanAccess(subject); > > Name change seems pointless It just happened because I computed it freshly above, and then didn't need it below. I can fix it to shrink the diff I guess, though it'll force a line break. > ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp > @@ +785,5 @@ > > > > // Now we have to set the right opener principal on the new window. Note > > // that we have to do this _before_ starting any URI loads, thanks to the > > + // sync nature of javascript: loads. > > + nsCOMPtr<nsIPrincipal> subjectPrincipal = nsContentUtils::GetSubjectPrincipal(); > > There's a null-check below Fixed. > > ::: layout/style/nsCSSStyleSheet.cpp > @@ -1632,5 @@ > > - nsresult rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal)); > > - NS_ENSURE_SUCCESS(rv, rv); > > - > > - if (!subjectPrincipal) { > > - return NS_ERROR_DOM_SECURITY_ERR; > > The number of places where we throw exceptions for null is... interesting. Given the title of this function and how it's used, the old behavior is almost certainly a bug. So I'm not going to fix this one, though I can if you really want me to.
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8408757 -
Attachment is obsolete: true
Attachment #8408759 -
Attachment is obsolete: true
Attachment #8408757 -
Flags: review?(Ms2ger)
Attachment #8410641 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8408760 -
Attachment is obsolete: true
Attachment #8408760 -
Flags: review?(Ms2ger)
Attachment #8410644 -
Flags: review?(Ms2ger)
Comment 12•11 years ago
|
||
Comment on attachment 8408758 [details] [diff] [review] Part 2 - Remove nsIScriptSecurityManager::GetCxSubjectPrincipal. v1 Review of attachment 8408758 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Do I own CAPS now? ::: content/base/src/nsContentUtils.cpp @@ +6104,5 @@ > nsContentUtils::GetContentSecurityPolicy(JSContext* aCx, > nsIContentSecurityPolicy** aCSP) > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + MOZ_ASSERT(aCx == GetCurrentJSContext()); Followup to remove the argument. ::: js/xpconnect/src/Sandbox.cpp @@ +216,2 @@ > if (NS_FAILED(rv)) > return false; (Is this an uncatchable exception?)
Attachment #8408758 -
Flags: review?(Ms2ger) → review+
Comment 13•11 years ago
|
||
Comment on attachment 8410641 [details] [diff] [review] Part 3 - Remove usage of nsIScriptSecurityManager::GetSubjectPrincipal. v2 Review of attachment 8410641 [details] [diff] [review]: ----------------------------------------------------------------- r=me, I think. ::: caps/src/nsScriptSecurityManager.cpp @@ +337,5 @@ > > ///////////////// Security Checks ///////////////// > > bool > nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx) Followup to remove the argument @@ +391,5 @@ > } > > NS_IMETHODIMP > nsScriptSecurityManager::CheckSameOrigin(JSContext* cx, > nsIURI* aTargetURI) Assert !cx || cx == GetCurrentContext() @@ +1104,5 @@ > } else { > strName.AppendLiteral("ForOrigin"); > } > nsXPIDLString errorMsg; > // We need to keep our existing failure rv and not override it Remove the comment ::: content/base/src/DOMParser.cpp @@ -400,5 @@ > - if (rv.Failed()) { > - return nullptr; > - } > - > - // We're called from JS; there better be a subject principal, really. Assert that, perhaps? ::: content/base/src/nsContentUtils.cpp @@ +1596,4 @@ > nsCOMPtr<nsIScriptObjectPrincipal> scriptObject = > do_QueryInterface(aWindow->IsOuterWindow() ? > aWindow->GetCurrentInnerWindow() : aWindow); > NS_ENSURE_TRUE(scriptObject, false); Does it matter that this changes behaviour? ::: dom/base/nsGlobalWindow.cpp @@ +6053,5 @@ > > // Try to get a host from the running principal -- this will do the > // right thing for javascript: and data: documents. > > nsresult rv = NS_OK; Move down ::: dom/events/DataTransfer.cpp @@ +629,1 @@ > NS_ENSURE_SUCCESS(rv, rv); This is dead ::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp @@ +1251,5 @@ > NS_IMETHODIMP > txMozillaXSLTProcessor::Initialize(nsISupports* aOwner, JSContext* cx, > JSObject* obj, const JS::CallArgs& args) > { > + return Init(nsContentUtils::GetSubjectPrincipal()); Assert GetCurrentContext() is not null? ::: security/manager/ssl/src/nsCrypto.cpp @@ -2050,5 @@ > - aRv.Throw(nrv); > - return nullptr; > - } > - if (MOZ_UNLIKELY(!principals)) { > - aRv.Throw(NS_ERROR_UNEXPECTED); Behaviour change... Maybe just assert we have a current context?
Attachment #8410641 -
Flags: review?(Ms2ger) → review+
Comment 14•11 years ago
|
||
Comment on attachment 8410644 [details] [diff] [review] Part 4 - Remove nsIScriptSecurityManager::GetSubjectPrincipal. v2 Review of attachment 8410644 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: caps/src/nsScriptSecurityManager.cpp @@ -1038,5 @@ > -nsIPrincipal* > -nsScriptSecurityManager::GetSubjectPrincipal(JSContext *cx, > - nsresult* rv) > -{ > - *rv = NS_OK; Β§#?!@# ::: content/base/src/nsContentUtils.cpp @@ +2334,5 @@ > // static > nsIPrincipal* > nsContentUtils::GetSubjectPrincipal() > { > + JSContext *cx = GetCurrentJSContext(); * to the left (three times in this function) @@ +2342,3 @@ > > + JSCompartment *compartment = js::GetContextCompartment(cx); > + MOZ_ASSERT(!!compartment); No need for the !!
Attachment #8410644 -
Flags: review?(Ms2ger) → review+
Comment 15•11 years ago
|
||
Comment on attachment 8408761 [details] [diff] [review] Part 5 - Cache the system principal on nsContentUtils and remove nsIScriptSecurityManager::SubjectPrincipalIsSystem. v1 Review of attachment 8408761 [details] [diff] [review]: ----------------------------------------------------------------- r=me. There's a bunch of places where we start supporting UniversalXPConnect and didn't before, though; I'd like bz to sign that off. ::: caps/src/nsScriptSecurityManager.cpp @@ +78,5 @@ > JSRuntime *nsScriptSecurityManager::sRuntime = 0; > bool nsScriptSecurityManager::sStrictFileOriginPolicy = true; > > bool > nsScriptSecurityManager::SubjectIsPrivileged() Followup to remove? ::: content/base/src/nsContentUtils.cpp @@ +380,5 @@ > return NS_ERROR_FAILURE; > NS_ADDREF(sSecurityManager); > > + sSecurityManager->GetSystemPrincipal(&sSystemPrincipal); > + if (!sSystemPrincipal) Assert @@ +4392,5 @@ > if (NS_SUCCEEDED((*aResourcePrincipal)->Subsumes(aExtraPrincipal, &subsumes)) && > subsumes) { > return false; > } > + NS_ADDREF(*aResourcePrincipal = sSystemPrincipal); Is this a leak? I think this is a leak. ::: js/xpconnect/wrappers/AccessCheck.cpp @@ +90,5 @@ > return isChrome(js::GetObjectCompartment(obj)); > } > > bool > AccessCheck::callerIsChrome() Followup to remove?
Attachment #8408761 -
Flags: superreview?
Attachment #8408761 -
Flags: review?(Ms2ger)
Attachment #8408761 -
Flags: review+
Updated•11 years ago
|
Attachment #8408761 -
Flags: superreview? → superreview?(bzbarsky)
Comment 16•11 years ago
|
||
Comment on attachment 8408761 [details] [diff] [review] Part 5 - Cache the system principal on nsContentUtils and remove nsIScriptSecurityManager::SubjectPrincipalIsSystem. v1 Treating UniversalXPConnect as identical to system across the board makes sense to me.
Attachment #8408761 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to :Ms2ger from comment #12) > Comment on attachment 8408758 [details] [diff] [review] > Part 2 - Remove nsIScriptSecurityManager::GetCxSubjectPrincipal. v1 > > Review of attachment 8408758 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me. Do I own CAPS now? > > ::: content/base/src/nsContentUtils.cpp > @@ +6104,5 @@ > > nsContentUtils::GetContentSecurityPolicy(JSContext* aCx, > > nsIContentSecurityPolicy** aCSP) > > { > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + MOZ_ASSERT(aCx == GetCurrentJSContext()); > > Followup to remove the argument. Filed bug 1006671. > ::: js/xpconnect/src/Sandbox.cpp > @@ +216,2 @@ > > if (NS_FAILED(rv)) > > return false; > > (Is this an uncatchable exception?) It depends on whether the XHR sets a JS exception or not.
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to :Ms2ger from comment #13) > > - // We're called from JS; there better be a subject principal, really. > > Assert that, perhaps? With these changes it's moot. There will always be a subject principal whether we're called from JS or not. > ::: content/base/src/nsContentUtils.cpp > @@ +1596,4 @@ > > nsCOMPtr<nsIScriptObjectPrincipal> scriptObject = > > do_QueryInterface(aWindow->IsOuterWindow() ? > > aWindow->GetCurrentInnerWindow() : aWindow); > > NS_ENSURE_TRUE(scriptObject, false); > > Does it matter that this changes behaviour? Unless I'm misunderstanding you, I don't think it matters. CanCallerAccess will always return true for the System Principla. > > ::: dom/base/nsGlobalWindow.cpp > @@ +6053,5 @@ > > > > // Try to get a host from the running principal -- this will do the > > // right thing for javascript: and data: documents. > > > > nsresult rv = NS_OK; > > Move down Done. > ::: dom/events/DataTransfer.cpp > @@ +629,1 @@ > > NS_ENSURE_SUCCESS(rv, rv); > > This is dead Fixed. > > ::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp > @@ +1251,5 @@ > > NS_IMETHODIMP > > txMozillaXSLTProcessor::Initialize(nsISupports* aOwner, JSContext* cx, > > JSObject* obj, const JS::CallArgs& args) > > { > > + return Init(nsContentUtils::GetSubjectPrincipal()); > > Assert GetCurrentContext() is not null? Ok. > ::: security/manager/ssl/src/nsCrypto.cpp > @@ -2050,5 @@ > > - aRv.Throw(nrv); > > - return nullptr; > > - } > > - if (MOZ_UNLIKELY(!principals)) { > > - aRv.Throw(NS_ERROR_UNEXPECTED); > > Behaviour change... Maybe just assert we have a current context? Ok.
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to :Ms2ger from comment #14) > ::: caps/src/nsScriptSecurityManager.cpp > @@ -1038,5 @@ > > -nsIPrincipal* > > -nsScriptSecurityManager::GetSubjectPrincipal(JSContext *cx, > > - nsresult* rv) > > -{ > > - *rv = NS_OK; > > Β§#?!@# I don't understand this. But since it's in removed code, I'm assuming it doesn't matter. > ::: content/base/src/nsContentUtils.cpp > @@ +2334,5 @@ > > // static > > nsIPrincipal* > > nsContentUtils::GetSubjectPrincipal() > > { > > + JSContext *cx = GetCurrentJSContext(); > > * to the left (three times in this function) Fixed. > > @@ +2342,3 @@ > > > > + JSCompartment *compartment = js::GetContextCompartment(cx); > > + MOZ_ASSERT(!!compartment); > > No need for the !! Fixed.
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to :Ms2ger from comment #15) > > bool > > nsScriptSecurityManager::SubjectIsPrivileged() Filed bug 1006692. > > Followup to remove? > > ::: content/base/src/nsContentUtils.cpp > > + sSecurityManager->GetSystemPrincipal(&sSystemPrincipal); > > + if (!sSystemPrincipal) > > Assert Ok. > > @@ +4392,5 @@ > > if (NS_SUCCEEDED((*aResourcePrincipal)->Subsumes(aExtraPrincipal, &subsumes)) && > > subsumes) { > > return false; > > } > > + NS_ADDREF(*aResourcePrincipal = sSystemPrincipal); > > Is this a leak? I think this is a leak. Good catch! I didn't see the bizarre COMPtr ptr. Fixed. > ::: js/xpconnect/wrappers/AccessCheck.cpp > @@ +90,5 @@ > > return isChrome(js::GetObjectCompartment(obj)); > > } > > > > bool > > AccessCheck::callerIsChrome() > > Followup to remove? Bug 1006692.
Comment 22•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #18) > (In reply to :Ms2ger from comment #13) > > > - // We're called from JS; there better be a subject principal, really. > > > > Assert that, perhaps? > > With these changes it's moot. There will always be a subject principal > whether we're called from JS or not. I was thinking to assert GetCurrentContext(). > > ::: content/base/src/nsContentUtils.cpp > > @@ +1596,4 @@ > > > nsCOMPtr<nsIScriptObjectPrincipal> scriptObject = > > > do_QueryInterface(aWindow->IsOuterWindow() ? > > > aWindow->GetCurrentInnerWindow() : aWindow); > > > NS_ENSURE_TRUE(scriptObject, false); > > > > Does it matter that this changes behaviour? > > Unless I'm misunderstanding you, I don't think it matters. CanCallerAccess > will always return true for the System Principla. It won't in this overload of CanCallerAccess; if scriptObject and GetCurrentContext() are both null, we used to return true, and now return false. Probably doesn't matter, though.
Assignee | ||
Comment 23•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd17d782f06a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1862e3f7ca6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbcc5e0e689 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d46b25e314d9
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd17d782f06a https://hg.mozilla.org/mozilla-central/rev/d1862e3f7ca6 https://hg.mozilla.org/mozilla-central/rev/ccbcc5e0e689 https://hg.mozilla.org/mozilla-central/rev/d46b25e314d9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•