From 1669cc677072fd78572a3ef76dd00454b326da47 Mon Sep 17 00:00:00 2001 From: Mirko Brodesser Date: Mon, 15 Jul 2019 13:03:33 +0200 Subject: [PATCH 1/5] Bug 1566046: rename `GetParentOrHostNode` to `GetParentOrShadowHostNode`. r=smaug Differential Revision: https://phabricator.services.mozilla.com/D38078 --- accessible/base/nsAccessibilityService.cpp | 5 +++-- dom/base/FragmentOrElement.cpp | 2 +- dom/base/nsINode.cpp | 2 +- dom/base/nsINode.h | 6 +----- dom/svg/SVGUseElement.cpp | 4 ++-- editor/libeditor/HTMLEditRules.cpp | 4 ++-- layout/base/PresShell.cpp | 2 +- layout/base/nsLayoutUtils.cpp | 6 +++--- 8 files changed, 14 insertions(+), 17 deletions(-) diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp index 0d6a59f95f5a..debe455458f1 100644 --- a/accessible/base/nsAccessibilityService.cpp +++ b/accessible/base/nsAccessibilityService.cpp @@ -1396,12 +1396,13 @@ nsAccessibilityService::CreateAccessibleByFrameType(nsIFrame* aFrame, if (table) { nsIContent* parentContent = - aContent->GetParentOrHostNode()->AsContent(); + aContent->GetParentOrShadowHostNode()->AsContent(); nsIFrame* parentFrame = nullptr; if (parentContent) { parentFrame = parentContent->GetPrimaryFrame(); if (!parentFrame || !parentFrame->IsTableWrapperFrame()) { - parentContent = parentContent->GetParentOrHostNode()->AsContent(); + parentContent = + parentContent->GetParentOrShadowHostNode()->AsContent(); if (parentContent) { parentFrame = parentContent->GetPrimaryFrame(); } diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp index 7ce14bfcdebc..b136d30e65ee 100644 --- a/dom/base/FragmentOrElement.cpp +++ b/dom/base/FragmentOrElement.cpp @@ -759,7 +759,7 @@ static nsINode* FindChromeAccessOnlySubtreeOwner(nsINode* aNode) { aNode = aNode->GetParentNode(); } - return aNode ? aNode->GetParentOrHostNode() : nullptr; + return aNode ? aNode->GetParentOrShadowHostNode() : nullptr; } already_AddRefed FindChromeAccessOnlySubtreeOwner( diff --git a/dom/base/nsINode.cpp b/dom/base/nsINode.cpp index 945af4713449..d3165c847968 100644 --- a/dom/base/nsINode.cpp +++ b/dom/base/nsINode.cpp @@ -261,7 +261,7 @@ nsINode* nsINode::GetRootNode(const GetRootNodeOptions& aOptions) { return SubtreeRoot(); } -nsINode* nsINode::GetParentOrHostNode() const { +nsINode* nsINode::GetParentOrShadowHostNode() const { if (mParent) { return mParent; } diff --git a/dom/base/nsINode.h b/dom/base/nsINode.h index 539f89881d8b..609cc3bea76c 100644 --- a/dom/base/nsINode.h +++ b/dom/base/nsINode.h @@ -884,11 +884,7 @@ class nsINode : public mozilla::dom::EventTarget { */ nsINode* GetParentNode() const { return mParent; } - /** - * This is similar to above, but in case 'this' is ShadowRoot, we return its - * host element. - */ - nsINode* GetParentOrHostNode() const; + nsINode* GetParentOrShadowHostNode() const; enum FlattenedParentType { eNotForStyle, eForStyle }; diff --git a/dom/svg/SVGUseElement.cpp b/dom/svg/SVGUseElement.cpp index 46ffde2ecd57..0f44d9286fad 100644 --- a/dom/svg/SVGUseElement.cpp +++ b/dom/svg/SVGUseElement.cpp @@ -244,8 +244,8 @@ bool SVGUseElement::IsCyclicReferenceTo(const Element& aTarget) const { if (mOriginal && mOriginal->IsCyclicReferenceTo(aTarget)) { return true; } - for (nsINode* parent = GetParentOrHostNode(); parent; - parent = parent->GetParentOrHostNode()) { + for (nsINode* parent = GetParentOrShadowHostNode(); parent; + parent = parent->GetParentOrShadowHostNode()) { if (parent == &aTarget) { return true; } diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 6cf04e512f8e..aab1f18e052e 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -10048,7 +10048,7 @@ nsresult HTMLEditRules::ConfirmSelectionInBody() { // XXXsmaug this code is insane. nsINode* temp = selectionStartPoint.GetContainer(); while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) { - temp = temp->GetParentOrHostNode(); + temp = temp->GetParentOrShadowHostNode(); } // If we aren't in the element, force the issue. @@ -10074,7 +10074,7 @@ nsresult HTMLEditRules::ConfirmSelectionInBody() { // XXXsmaug this code is insane. temp = selectionEndPoint.GetContainer(); while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) { - temp = temp->GetParentOrHostNode(); + temp = temp->GetParentOrShadowHostNode(); } // If we aren't in the element, force the issue. diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp index 166cf23893b1..046e409159de 100644 --- a/layout/base/PresShell.cpp +++ b/layout/base/PresShell.cpp @@ -4725,7 +4725,7 @@ UniquePtr PresShell::CreateRangePaintInfo( break; } - ancestor = ancestor->GetParentOrHostNode(); + ancestor = ancestor->GetParentOrShadowHostNode(); } // use the nearest ancestor frame that includes all continuations as the diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp index a9d82a00947f..071732588f34 100644 --- a/layout/base/nsLayoutUtils.cpp +++ b/layout/base/nsLayoutUtils.cpp @@ -1669,7 +1669,7 @@ int32_t nsLayoutUtils::DoCompareTreePosition( AutoTArray content1Ancestors; nsINode* c1; for (c1 = aContent1; c1 && c1 != aCommonAncestor; - c1 = c1->GetParentOrHostNode()) { + c1 = c1->GetParentOrShadowHostNode()) { content1Ancestors.AppendElement(c1); } if (!c1 && aCommonAncestor) { @@ -1681,7 +1681,7 @@ int32_t nsLayoutUtils::DoCompareTreePosition( AutoTArray content2Ancestors; nsINode* c2; for (c2 = aContent2; c2 && c2 != aCommonAncestor; - c2 = c2->GetParentOrHostNode()) { + c2 = c2->GetParentOrShadowHostNode()) { content2Ancestors.AppendElement(c2); } if (!c2 && aCommonAncestor) { @@ -1718,7 +1718,7 @@ int32_t nsLayoutUtils::DoCompareTreePosition( // content1Ancestor != content2Ancestor, so they must be siblings with the // same parent - nsINode* parent = content1Ancestor->GetParentOrHostNode(); + nsINode* parent = content1Ancestor->GetParentOrShadowHostNode(); #ifdef DEBUG // TODO: remove the uglyness, see bug 598468. NS_ASSERTION(gPreventAssertInCompareTreePosition || parent, From 8135a5859fc48473ac4e25aaa9e95d18bcdef788 Mon Sep 17 00:00:00 2001 From: Mirko Brodesser Date: Mon, 15 Jul 2019 11:56:00 +0200 Subject: [PATCH 2/5] Bug 1566046: move `nsContentUtils::ContentIsShadowIncludingDescendantOf` to `nsINode::IsShadowIncludingInclusiveDescendantOf`. r=smaug Differential Revision: https://phabricator.services.mozilla.com/D38079 --- dom/base/FragmentOrElement.cpp | 4 +-- dom/base/nsContentUtils.cpp | 28 +------------------ dom/base/nsContentUtils.h | 7 ----- dom/base/nsINode.cpp | 20 +++++++++++++ dom/base/nsINode.h | 7 +++++ .../spellcheck/src/mozInlineSpellChecker.cpp | 9 ++---- 6 files changed, 33 insertions(+), 42 deletions(-) diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp index b136d30e65ee..6171bf01e4f1 100644 --- a/dom/base/FragmentOrElement.cpp +++ b/dom/base/FragmentOrElement.cpp @@ -990,8 +990,8 @@ void nsIContent::GetEventTargetParent(EventChainPreVisitor& aVisitor) { // dispatching event to Window object in a content page and // propagating the event to a chrome Element. if (targetInKnownToBeHandledScope && - nsContentUtils::ContentIsShadowIncludingDescendantOf( - this, targetInKnownToBeHandledScope->SubtreeRoot())) { + IsShadowIncludingInclusiveDescendantOf( + targetInKnownToBeHandledScope->SubtreeRoot())) { // Part of step 11.4. // "If target's root is a shadow-including inclusive ancestor of // parent, then" diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index 1e72f01da338..e791ed474704 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -2200,32 +2200,6 @@ bool nsContentUtils::ContentIsHostIncludingDescendantOf( return false; } -bool nsContentUtils::ContentIsShadowIncludingDescendantOf( - const nsINode* aPossibleDescendant, const nsINode* aPossibleAncestor) { - MOZ_ASSERT(aPossibleDescendant, "The possible descendant is null!"); - MOZ_ASSERT(aPossibleAncestor, "The possible ancestor is null!"); - - if (aPossibleAncestor == aPossibleDescendant->GetComposedDoc()) { - return true; - } - - do { - if (aPossibleDescendant == aPossibleAncestor) { - return true; - } - - if (aPossibleDescendant->NodeType() == nsINode::DOCUMENT_FRAGMENT_NODE) { - ShadowRoot* shadowRoot = - ShadowRoot::FromNode(const_cast(aPossibleDescendant)); - aPossibleDescendant = shadowRoot ? shadowRoot->GetHost() : nullptr; - } else { - aPossibleDescendant = aPossibleDescendant->GetParentNode(); - } - } while (aPossibleDescendant); - - return false; -} - // static bool nsContentUtils::ContentIsCrossDocDescendantOf(nsINode* aPossibleDescendant, nsINode* aPossibleAncestor) { @@ -2285,7 +2259,7 @@ nsINode* nsContentUtils::Retarget(nsINode* aTargetA, nsINode* aTargetB) { } // or A's root is a shadow-including inclusive ancestor of B... - if (nsContentUtils::ContentIsShadowIncludingDescendantOf(aTargetB, root)) { + if (aTargetB->IsShadowIncludingInclusiveDescendantOf(root)) { // ...then return A. return aTargetA; } diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h index 7162a83ec429..3664849ece7a 100644 --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ -344,13 +344,6 @@ class nsContentUtils { static bool ContentIsHostIncludingDescendantOf( const nsINode* aPossibleDescendant, const nsINode* aPossibleAncestor); - /** - * Similar to above, but does special case only ShadowRoot, - * not HTMLTemplateElement. - */ - static bool ContentIsShadowIncludingDescendantOf( - const nsINode* aPossibleDescendant, const nsINode* aPossibleAncestor); - /** * Similar to nsINode::IsInclusiveDescendantOf except it crosses document * boundaries, this function uses ancestor/descendant relations in the diff --git a/dom/base/nsINode.cpp b/dom/base/nsINode.cpp index d3165c847968..d70f288a977a 100644 --- a/dom/base/nsINode.cpp +++ b/dom/base/nsINode.cpp @@ -131,6 +131,26 @@ bool nsINode::IsInclusiveDescendantOf(const nsINode* aNode) const { return false; } +bool nsINode::IsShadowIncludingInclusiveDescendantOf( + const nsINode* aNode) const { + MOZ_ASSERT(aNode, "The node is nullptr."); + + if (this->GetComposedDoc() == aNode) { + return true; + } + + const nsINode* node = this; + do { + if (node == aNode) { + return true; + } + + node = node->GetParentOrShadowHostNode(); + } while (node); + + return false; +} + nsINode::nsSlots::nsSlots() : mWeakReference(nullptr) {} nsINode::nsSlots::~nsSlots() { diff --git a/dom/base/nsINode.h b/dom/base/nsINode.h index 609cc3bea76c..3c15c375e795 100644 --- a/dom/base/nsINode.h +++ b/dom/base/nsINode.h @@ -424,6 +424,13 @@ class nsINode : public mozilla::dom::EventTarget { */ bool IsInclusiveDescendantOf(const nsINode* aNode) const; + /** + * https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-descendant + * + * @param aNode must not be nullptr. + */ + bool IsShadowIncludingInclusiveDescendantOf(const nsINode* aNode) const; + /** * Return this node as a document fragment. Asserts IsDocumentFragment(). * diff --git a/extensions/spellcheck/src/mozInlineSpellChecker.cpp b/extensions/spellcheck/src/mozInlineSpellChecker.cpp index de5ce5e6dc59..91985115020b 100644 --- a/extensions/spellcheck/src/mozInlineSpellChecker.cpp +++ b/extensions/spellcheck/src/mozInlineSpellChecker.cpp @@ -222,8 +222,7 @@ nsresult mozInlineSpellStatus::InitForNavigation( } // the anchor node might not be in the DOM anymore, check if (root && aOldAnchorNode && - !nsContentUtils::ContentIsShadowIncludingDescendantOf(aOldAnchorNode, - root)) { + !aOldAnchorNode->IsShadowIncludingInclusiveDescendantOf(root)) { *aContinue = false; return NS_OK; } @@ -1271,10 +1270,8 @@ nsresult mozInlineSpellChecker::DoSpellCheck( // aWordUtil.GetRootNode() nsINode* rootNode = aWordUtil.GetRootNode(); if (!beginNode->IsInComposedDoc() || !endNode->IsInComposedDoc() || - !nsContentUtils::ContentIsShadowIncludingDescendantOf(beginNode, - rootNode) || - !nsContentUtils::ContentIsShadowIncludingDescendantOf(endNode, - rootNode)) { + !beginNode->IsShadowIncludingInclusiveDescendantOf(rootNode) || + !endNode->IsShadowIncludingInclusiveDescendantOf(rootNode)) { // Just bail out and don't try to spell-check this return NS_OK; } From 18c49c3c24e8f3dd40a1e7c9cffdddcfdad7f81b Mon Sep 17 00:00:00 2001 From: Ryan Hunt Date: Sun, 7 Jul 2019 11:02:11 -0500 Subject: [PATCH 3/5] Bug 1562493 - Clear out pointer to parent BrowserBridgeParent or BrowserHost when strong-ref from parent to child is released. r=nika The ipc::browser-destroyed message must be sent when a top-level remote browser is destroyed. Currently this is done by BrowserParent::ActoryDestroy and relies on having access to mBrowserHost. This can break if we start clearing out this pointer. This commit moves the trigger to fire in BrowserHost::DestroyComplete. Semantically this is different, but from the users of this observer, I don't see any risk. Differential Revision: https://phabricator.services.mozilla.com/D37171 --HG-- extra : rebase_source : c863fb8d8a3a540d3dec42472c71342bed8ba541 extra : amend_source : aae3d054d453e63c10fec8707c8cda0497580a87 --- dom/ipc/BrowserBridgeParent.cpp | 1 + dom/ipc/BrowserHost.cpp | 9 +++++++++ dom/ipc/BrowserParent.cpp | 23 ++++++++--------------- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/dom/ipc/BrowserBridgeParent.cpp b/dom/ipc/BrowserBridgeParent.cpp index c8d8c86af78a..eeac184dd651 100644 --- a/dom/ipc/BrowserBridgeParent.cpp +++ b/dom/ipc/BrowserBridgeParent.cpp @@ -114,6 +114,7 @@ BrowserParent* BrowserBridgeParent::Manager() { void BrowserBridgeParent::Destroy() { if (mBrowserParent) { mBrowserParent->Destroy(); + mBrowserParent->SetBrowserBridgeParent(nullptr); mBrowserParent = nullptr; } } diff --git a/dom/ipc/BrowserHost.cpp b/dom/ipc/BrowserHost.cpp index 60e909a0431c..afcb4ef92f63 100644 --- a/dom/ipc/BrowserHost.cpp +++ b/dom/ipc/BrowserHost.cpp @@ -10,6 +10,8 @@ #include "mozilla/dom/CancelContentJSOptionsBinding.h" #include "mozilla/dom/WindowGlobalParent.h" +#include "nsIObserverService.h" + namespace mozilla { namespace dom { @@ -68,7 +70,14 @@ void BrowserHost::DestroyComplete() { } mRoot->SetOwnerElement(nullptr); mRoot->Destroy(); + mRoot->SetBrowserHost(nullptr); mRoot = nullptr; + + nsCOMPtr os = services::GetObserverService(); + if (os) { + os->NotifyObservers(NS_ISUPPORTS_CAST(nsIRemoteTab*, this), + "ipc:browser-destroyed", nullptr); + } } bool BrowserHost::Show(const ScreenIntSize& aSize, bool aParentIsActive) { diff --git a/dom/ipc/BrowserParent.cpp b/dom/ipc/BrowserParent.cpp index bcbe48808f8f..d00656f6d788 100644 --- a/dom/ipc/BrowserParent.cpp +++ b/dom/ipc/BrowserParent.cpp @@ -675,7 +675,6 @@ void BrowserParent::ActorDestroy(ActorDestroyReason why) { // Tell our embedder that the tab is now going away unless we're an // out-of-process iframe. RefPtr frameLoader = GetFrameLoader(true); - nsCOMPtr os = services::GetObserverService(); if (frameLoader) { ReceiveMessage(CHILD_PROCESS_SHUTDOWN_MESSAGE, false, nullptr, nullptr, nullptr); @@ -694,10 +693,6 @@ void BrowserParent::ActorDestroy(ActorDestroyReason why) { } mFrameLoader = nullptr; - if (os && mBrowserHost) { - os->NotifyObservers(NS_ISUPPORTS_CAST(nsIRemoteTab*, mBrowserHost), - "ipc:browser-destroyed", nullptr); - } } mozilla::ipc::IPCResult BrowserParent::RecvMoveFocus( @@ -3711,20 +3706,18 @@ void BrowserParent::LiveResizeStarted() { SuppressDisplayport(true); } void BrowserParent::LiveResizeStopped() { SuppressDisplayport(false); } void BrowserParent::SetBrowserBridgeParent(BrowserBridgeParent* aBrowser) { - // We should not have either a browser bridge or browser host yet - MOZ_ASSERT(!mBrowserBridgeParent); - MOZ_ASSERT(!mBrowserHost); - // We should not have owner content yet - MOZ_ASSERT(!mFrameElement); + // We should either be clearing out our reference to a browser bridge, or not + // have either a browser bridge, browser host, or owner content yet. + MOZ_ASSERT(!aBrowser || + (!mBrowserBridgeParent && !mBrowserHost && !mFrameElement)); mBrowserBridgeParent = aBrowser; } void BrowserParent::SetBrowserHost(BrowserHost* aBrowser) { - // We should not have either a browser bridge or browser host yet - MOZ_ASSERT(!mBrowserBridgeParent); - MOZ_ASSERT(!mBrowserHost); - // We should not have owner content yet - MOZ_ASSERT(!mFrameElement); + // We should either be clearing out our reference to a browser host, or not + // have either a browser bridge, browser host, or owner content yet. + MOZ_ASSERT(!aBrowser || + (!mBrowserBridgeParent && !mBrowserHost && !mFrameElement)); mBrowserHost = aBrowser; } From 18e9f3ba804eb5b0e53ae2d50fbee37a5eeb3ed4 Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Thu, 11 Jul 2019 13:48:28 -0700 Subject: [PATCH 4/5] bug 1564481 - reset HSTS/HPKP state to factory settings rather than storing knockout entries for preloaded sites r=jcj r=KevinJacobs As originally implemented, nsISiteSecurityService.removeState allowed direct access to remove HSTS state. It also provided the implementation for when the browser encountered an HSTS header with "max-age=0". In bug 775370, it was updated to store an entry that would override preloaded information when processing such headers. However, this meant that the semantics of the direct access API had changed. Preloaded information could be overridden if a user invoked the "forget about this site" feature. This change fixes the public API (and renames it to "resetState") so it actually behaves as its consumers expect. Reviewers: jcj!, KevinJacobs! Tags: #secure-revision Bug #: 1564481 Differential Revision: https://phabricator.services.mozilla.com/D38108 --HG-- extra : rebase_source : 8dd5460d3fd3c0ce92746cc83fae220d6e2a83cf extra : amend_source : 171ebb015e9f9ae775f0caa22e161d41970f3d51 --- .../content/test/general/browser_blockHPKP.js | 2 +- .../content/test/general/browser_star_hsts.js | 2 +- .../test/test_network_security-hpkp.html | 2 +- .../test/test_network_security-hsts.html | 2 +- .../manager/ssl/nsISiteSecurityService.idl | 15 ++- .../manager/ssl/nsSiteSecurityService.cpp | 96 ++++++++----- security/manager/ssl/nsSiteSecurityService.h | 10 +- .../browser/browser_bug627234_perwindowpb.js | 2 +- .../ssl/tests/unit/test_ocsp_must_staple.js | 2 +- .../tests/unit/test_pinning_header_parsing.js | 2 +- .../tests/unit/test_sss_originAttributes.js | 6 +- .../ssl/tests/unit/test_sss_resetState.js | 126 ++++++++++++++++++ .../manager/ssl/tests/unit/test_sts_fqdn.js | 2 +- security/manager/ssl/tests/unit/xpcshell.ini | 1 + .../content/SpecialPowersAPIParent.jsm | 2 +- .../components/cleardata/ClearDataService.jsm | 6 +- 16 files changed, 220 insertions(+), 58 deletions(-) create mode 100644 security/manager/ssl/tests/unit/test_sss_resetState.js diff --git a/browser/base/content/test/general/browser_blockHPKP.js b/browser/base/content/test/general/browser_blockHPKP.js index da3cf8aedc82..8526353ecb18 100644 --- a/browser/base/content/test/general/browser_blockHPKP.js +++ b/browser/base/content/test/general/browser_blockHPKP.js @@ -43,7 +43,7 @@ function test() { Services.prefs.clearUserPref(kpkpEnforcementPref); Services.prefs.clearUserPref(khpkpPinninEnablePref); let uri = Services.io.newURI("https://" + kPinningDomain); - gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); + gSSService.resetState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); }); whenNewTabLoaded(window, loadPinningPage); } diff --git a/browser/base/content/test/general/browser_star_hsts.js b/browser/base/content/test/general/browser_star_hsts.js index c8ac3cace632..f609e9d86d59 100644 --- a/browser/base/content/test/general/browser_star_hsts.js +++ b/browser/base/content/test/general/browser_star_hsts.js @@ -14,7 +14,7 @@ add_task(async function test_star_redirect() { let sss = Cc["@mozilla.org/ssservice;1"].getService( Ci.nsISiteSecurityService ); - sss.removeState( + sss.resetState( Ci.nsISiteSecurityService.HEADER_HSTS, NetUtil.newURI("http://example.com/"), 0 diff --git a/devtools/shared/webconsole/test/test_network_security-hpkp.html b/devtools/shared/webconsole/test/test_network_security-hpkp.html index a9ff1616ae74..11978032dc93 100644 --- a/devtools/shared/webconsole/test/test_network_security-hpkp.html +++ b/devtools/shared/webconsole/test/test_network_security-hpkp.html @@ -54,7 +54,7 @@ function startTest() { .getService(Ci.nsIIOService); for (let {url} of TEST_CASES) { let uri = gIOService.newURI(url); - gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); + gSSService.resetState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); } }); diff --git a/devtools/shared/webconsole/test/test_network_security-hsts.html b/devtools/shared/webconsole/test/test_network_security-hsts.html index 394fa68ff0f3..0c5e6030d53b 100644 --- a/devtools/shared/webconsole/test/test_network_security-hsts.html +++ b/devtools/shared/webconsole/test/test_network_security-hsts.html @@ -48,7 +48,7 @@ function startTest() .getService(Ci.nsIIOService); let uri = gIOService.newURI(TEST_CASES[0].url); - gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0); + gSSService.resetState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0); }); info("Test detection of HTTP Strict Transport Security."); diff --git a/security/manager/ssl/nsISiteSecurityService.idl b/security/manager/ssl/nsISiteSecurityService.idl index 6da61f13c2a9..54d9117af021 100644 --- a/security/manager/ssl/nsISiteSecurityService.idl +++ b/security/manager/ssl/nsISiteSecurityService.idl @@ -160,10 +160,13 @@ interface nsISiteSecurityService : nsISupports [optional] out uint32_t aFailureResult); /** - * Given a header type, removes state relating to that header of a host, + * Given a header type, resets state relating to that header of a host, * including the includeSubdomains state that would affect subdomains. * This essentially removes the state for the domain tree rooted at this - * host. + * host. If any preloaded information is present for that host, that + * information will then be used instead of any other previously existing + * state. + * * @param aType the type of security state in question * @param aURI the URI of the target host * @param aFlags options for this request as defined in nsISocketProvider: @@ -175,10 +178,10 @@ interface nsISiteSecurityService : nsISupports * happens). */ [implicit_jscontext, optional_argc, must_use] - void removeState(in uint32_t aType, - in nsIURI aURI, - in uint32_t aFlags, - [optional] in jsval aOriginAttributes); + void resetState(in uint32_t aType, + in nsIURI aURI, + in uint32_t aFlags, + [optional] in jsval aOriginAttributes); /** * Checks whether or not the URI's hostname has a given security state set. diff --git a/security/manager/ssl/nsSiteSecurityService.cpp b/security/manager/ssl/nsSiteSecurityService.cpp index 0ab01b32fc20..71c334c01f7f 100644 --- a/security/manager/ssl/nsSiteSecurityService.cpp +++ b/security/manager/ssl/nsSiteSecurityService.cpp @@ -557,11 +557,12 @@ nsresult nsSiteSecurityService::SetHSTSState( SecurityPropertySource aSource, const OriginAttributes& aOriginAttributes) { nsAutoCString hostname(aHost); bool isPreload = (aSource == SourcePreload); - // If max-age is zero, that's an indication to immediately remove the - // security state, so here's a shortcut. - if (!maxage) { - return RemoveStateInternal(aType, hostname, flags, isPreload, - aOriginAttributes); + // If max-age is zero, the host is no longer considered HSTS. If the host was + // preloaded, we store an entry indicating that this host is not HSTS, causing + // the preloaded information to be ignored. + if (maxage == 0) { + return MarkHostAsNotHSTS(aType, hostname, flags, isPreload, + aOriginAttributes); } MOZ_ASSERT( @@ -608,28 +609,17 @@ nsresult nsSiteSecurityService::SetHSTSState( return NS_OK; } -nsresult nsSiteSecurityService::RemoveStateInternal( - uint32_t aType, nsIURI* aURI, uint32_t aFlags, - const OriginAttributes& aOriginAttributes) { - nsAutoCString hostname; - GetHost(aURI, hostname); - return RemoveStateInternal(aType, hostname, aFlags, false, aOriginAttributes); -} - -nsresult nsSiteSecurityService::RemoveStateInternal( +// Helper function to mark a host as not HSTS. In the general case, we can just +// remove the HSTS state. However, for preloaded entries, we have to store an +// entry that indicates this host is not HSTS to prevent the implementation +// using the preloaded information. +nsresult nsSiteSecurityService::MarkHostAsNotHSTS( uint32_t aType, const nsAutoCString& aHost, uint32_t aFlags, bool aIsPreload, const OriginAttributes& aOriginAttributes) { - // Child processes are not allowed direct access to this. - if (!XRE_IsParentProcess()) { - MOZ_CRASH( - "Child process: no direct access to " - "nsISiteSecurityService::RemoveStateInternal"); + // This only applies to HSTS. + if (aType != nsISiteSecurityService::HEADER_HSTS) { + return NS_ERROR_INVALID_ARG; } - - // Only HSTS is supported at the moment. - NS_ENSURE_TRUE(aType == nsISiteSecurityService::HEADER_HSTS || - aType == nsISiteSecurityService::HEADER_HPKP, - NS_ERROR_NOT_IMPLEMENTED); if (aIsPreload && aOriginAttributes != OriginAttributes()) { return NS_ERROR_INVALID_ARG; } @@ -638,7 +628,6 @@ nsresult nsSiteSecurityService::RemoveStateInternal( mozilla::DataStorageType storageType = isPrivate ? mozilla::DataStorage_Private : mozilla::DataStorage_Persistent; - // If this host is in the preload list, we have to store a knockout entry. nsAutoCString storageKey; SetStorageKey(aHost, aType, aOriginAttributes, storageKey); @@ -675,10 +664,18 @@ nsresult nsSiteSecurityService::RemoveStateInternal( } NS_IMETHODIMP -nsSiteSecurityService::RemoveState(uint32_t aType, nsIURI* aURI, - uint32_t aFlags, - JS::HandleValue aOriginAttributes, - JSContext* aCx, uint8_t aArgc) { +nsSiteSecurityService::ResetState(uint32_t aType, nsIURI* aURI, uint32_t aFlags, + JS::HandleValue aOriginAttributes, + JSContext* aCx, uint8_t aArgc) { + if (!XRE_IsParentProcess()) { + MOZ_CRASH( + "Child process: no direct access to " + "nsISiteSecurityService::ResetState"); + } + if (!aURI) { + return NS_ERROR_INVALID_ARG; + } + OriginAttributes originAttributes; if (aArgc > 0) { // OriginAttributes were passed in. @@ -687,7 +684,39 @@ nsSiteSecurityService::RemoveState(uint32_t aType, nsIURI* aURI, return NS_ERROR_INVALID_ARG; } } - return RemoveStateInternal(aType, aURI, aFlags, originAttributes); + + return ResetStateInternal(aType, aURI, aFlags, originAttributes); +} + +// Helper function to reset stored state of the given type for the host +// identified by the given URI. If there is preloaded information for the host, +// that information will be used for future queries. C.f. MarkHostAsNotHSTS, +// which will store a knockout entry for preloaded HSTS hosts that have sent a +// header with max-age=0 (meaning preloaded information will then not be used +// for that host). +nsresult nsSiteSecurityService::ResetStateInternal( + uint32_t aType, nsIURI* aURI, uint32_t aFlags, + const OriginAttributes& aOriginAttributes) { + if (!aURI) { + return NS_ERROR_INVALID_ARG; + } + if (aType != nsISiteSecurityService::HEADER_HSTS && + aType != nsISiteSecurityService::HEADER_HPKP) { + return NS_ERROR_INVALID_ARG; + } + nsAutoCString hostname; + nsresult rv = GetHost(aURI, hostname); + if (NS_FAILED(rv)) { + return rv; + } + nsAutoCString storageKey; + SetStorageKey(hostname, aType, aOriginAttributes, storageKey); + bool isPrivate = aFlags & nsISocketProvider::NO_PERMANENT_STORAGE; + mozilla::DataStorageType storageType = isPrivate + ? mozilla::DataStorage_Private + : mozilla::DataStorage_Persistent; + mSiteStateStorage->Remove(storageKey, storageType); + return NS_OK; } static bool HostIsIPAddress(const nsCString& hostname) { @@ -1062,9 +1091,12 @@ nsresult nsSiteSecurityService::ProcessPKPHeader( return NS_ERROR_FAILURE; } - // if maxAge == 0 we must delete all state, for now no hole-punching + // If maxAge == 0, we remove dynamic HPKP state for this host. Due to + // architectural constraints, if this host was preloaded, any future lookups + // will use the preloaded state (i.e. we can't store a "this host is not HPKP" + // entry like we can for HSTS). if (maxAge == 0) { - return RemoveStateInternal(aType, aSourceURI, aFlags, aOriginAttributes); + return ResetStateInternal(aType, aSourceURI, aFlags, aOriginAttributes); } // clamp maxAge to the maximum set by pref diff --git a/security/manager/ssl/nsSiteSecurityService.h b/security/manager/ssl/nsSiteSecurityService.h index f9ff2759097e..8b4be4b3a7ef 100644 --- a/security/manager/ssl/nsSiteSecurityService.h +++ b/security/manager/ssl/nsSiteSecurityService.h @@ -191,11 +191,11 @@ class nsSiteSecurityService : public nsISiteSecurityService, nsresult SetHPKPState(const char* aHost, SiteHPKPState& entry, uint32_t flags, bool aIsPreload, const OriginAttributes& aOriginAttributes); - nsresult RemoveStateInternal(uint32_t aType, nsIURI* aURI, uint32_t aFlags, - const OriginAttributes& aOriginAttributes); - nsresult RemoveStateInternal(uint32_t aType, const nsAutoCString& aHost, - uint32_t aFlags, bool aIsPreload, - const OriginAttributes& aOriginAttributes); + nsresult MarkHostAsNotHSTS(uint32_t aType, const nsAutoCString& aHost, + uint32_t aFlags, bool aIsPreload, + const OriginAttributes& aOriginAttributes); + nsresult ResetStateInternal(uint32_t aType, nsIURI* aURI, uint32_t aFlags, + const OriginAttributes& aOriginAttributes); bool HostHasHSTSEntry(const nsAutoCString& aHost, bool aRequireIncludeSubdomains, uint32_t aFlags, const OriginAttributes& aOriginAttributes, diff --git a/security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js b/security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js index 6dd3ec1eb4f9..0ed800cc9a0a 100644 --- a/security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js +++ b/security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js @@ -93,7 +93,7 @@ function test() { aWin.close(); }); uri = Services.io.newURI("http://localhost"); - gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0); + gSSService.resetState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0); }); // test first when on private mode diff --git a/security/manager/ssl/tests/unit/test_ocsp_must_staple.js b/security/manager/ssl/tests/unit/test_ocsp_must_staple.js index ba6f81b86999..56aec21674d4 100644 --- a/security/manager/ssl/tests/unit/test_ocsp_must_staple.js +++ b/security/manager/ssl/tests/unit/test_ocsp_must_staple.js @@ -69,7 +69,7 @@ function add_tests() { ); // Clear accumulated state. - ssservice.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); + ssservice.resetState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); Services.prefs.clearUserPref( "security.cert_pinning.process_headers_from_non_builtin_roots" ); diff --git a/security/manager/ssl/tests/unit/test_pinning_header_parsing.js b/security/manager/ssl/tests/unit/test_pinning_header_parsing.js index 0b717f8b8be5..d1859436e454 100644 --- a/security/manager/ssl/tests/unit/test_pinning_header_parsing.js +++ b/security/manager/ssl/tests/unit/test_pinning_header_parsing.js @@ -55,7 +55,7 @@ function checkPassValidPin(pinValue, settingPin, expectedMaxAge) { // setup preconditions for the test, if setting ensure there is no previous // state, if removing ensure there is a valid pin in place. if (settingPin) { - gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); + gSSService.resetState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); } else { // add a known valid pin! let validPinValue = "max-age=5000;" + VALID_PIN1 + BACKUP_PIN1; diff --git a/security/manager/ssl/tests/unit/test_sss_originAttributes.js b/security/manager/ssl/tests/unit/test_sss_originAttributes.js index 289809183ffc..a1de2edb29e5 100644 --- a/security/manager/ssl/tests/unit/test_sss_originAttributes.js +++ b/security/manager/ssl/tests/unit/test_sss_originAttributes.js @@ -83,7 +83,7 @@ function doTest(originAttributes1, originAttributes2, shouldShare) { if (!shouldShare) { // Remove originAttributes2 from the storage. - sss.removeState(type, uri, 0, originAttributes2); + sss.resetState(type, uri, 0, originAttributes2); ok( sss.isSecureURI(type, uri, 0, originAttributes1), "URI should still be secure given original origin attributes" @@ -91,7 +91,7 @@ function doTest(originAttributes1, originAttributes2, shouldShare) { } // Remove originAttributes1 from the storage. - sss.removeState(type, uri, 0, originAttributes1); + sss.resetState(type, uri, 0, originAttributes1); ok( !sss.isSecureURI(type, uri, 0, originAttributes1), "URI should be not be secure after removeState" @@ -161,7 +161,7 @@ function testInvalidOriginAttributes(originAttributes) { originAttributes ), () => sss.isSecureURI(type, uri, 0, originAttributes), - () => sss.removeState(type, uri, 0, originAttributes), + () => sss.resetState(type, uri, 0, originAttributes), ]; for (let callback of callbacks) { diff --git a/security/manager/ssl/tests/unit/test_sss_resetState.js b/security/manager/ssl/tests/unit/test_sss_resetState.js new file mode 100644 index 000000000000..b3849689468c --- /dev/null +++ b/security/manager/ssl/tests/unit/test_sss_resetState.js @@ -0,0 +1,126 @@ +// -*- indent-tabs-mode: nil; js-indent-level: 2 -*- +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +"use strict"; + +// Tests that resetting HSTS/HPKP state in the way the "forget about this site" +// functionality does works as expected for preloaded and non-preloaded sites. + +do_get_profile(); + +var gCertDB = Cc["@mozilla.org/security/x509certdb;1"].getService( + Ci.nsIX509CertDB +); +const ROOT_CERT = addCertFromFile(gCertDB, "bad_certs/test-ca.pem", "CTu,,"); + +var gSSService = Cc["@mozilla.org/ssservice;1"].getService( + Ci.nsISiteSecurityService +); + +function run_test() { + Services.prefs.setBoolPref( + "security.cert_pinning.process_headers_from_non_builtin_roots", + true + ); + test_removeState(Ci.nsISiteSecurityService.HEADER_HSTS, 0); + test_removeState( + Ci.nsISiteSecurityService.HEADER_HSTS, + Ci.nsISocketProvider.NO_PERMANENT_STORAGE + ); + test_removeState(Ci.nsISiteSecurityService.HEADER_HPKP, 0); + test_removeState( + Ci.nsISiteSecurityService.HEADER_HPKP, + Ci.nsISocketProvider.NO_PERMANENT_STORAGE + ); +} + +function test_removeState(type, flags) { + info(`running test_removeState(type=${type}, flags=${flags})`); + const NON_ISSUED_KEY_HASH = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; + const PINNING_ROOT_KEY_HASH = "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8="; + const PINNING_HEADERS = `pin-sha256="${NON_ISSUED_KEY_HASH}"; pin-sha256="${PINNING_ROOT_KEY_HASH}"`; + let headerAddendum = + type == Ci.nsISiteSecurityService.HEADER_HPKP ? PINNING_HEADERS : ""; + let secInfo = new FakeTransportSecurityInfo( + constructCertFromFile("bad_certs/default-ee.pem") + ); + // Simulate visiting a non-preloaded site by processing an HSTS or HPKP header + // (depending on which type we were given), check that the HSTS/HPKP bit gets + // set, simulate "forget about this site" (call removeState), and then check + // that the HSTS/HPKP bit isn't set. + let notPreloadedURI = Services.io.newURI("https://not-preloaded.example.com"); + ok(!gSSService.isSecureURI(type, notPreloadedURI, flags)); + gSSService.processHeader( + type, + notPreloadedURI, + "max-age=1000;" + headerAddendum, + secInfo, + flags, + Ci.nsISiteSecurityService.SOURCE_ORGANIC_REQUEST + ); + ok(gSSService.isSecureURI(type, notPreloadedURI, flags)); + gSSService.resetState(type, notPreloadedURI, flags); + ok(!gSSService.isSecureURI(type, notPreloadedURI, flags)); + + // Simulate visiting a non-preloaded site that unsets HSTS/HPKP by processing + // an HSTS/HPKP header with "max-age=0", check that the HSTS/HPKP bit isn't + // set, simulate "forget about this site" (call removeState), and then check + // that the HSTS/HPKP bit isn't set. + gSSService.processHeader( + type, + notPreloadedURI, + "max-age=0;" + headerAddendum, + secInfo, + flags, + Ci.nsISiteSecurityService.SOURCE_ORGANIC_REQUEST + ); + ok(!gSSService.isSecureURI(type, notPreloadedURI, flags)); + gSSService.resetState(type, notPreloadedURI, flags); + ok(!gSSService.isSecureURI(type, notPreloadedURI, flags)); + + // Simulate visiting a preloaded site by processing an HSTS/HPKP header, check + // that the HSTS/HPKP bit is still set, simulate "forget about this site" + // (call removeState), and then check that the HSTS/HPKP bit is still set. + let preloadedHost = + type == Ci.nsISiteSecurityService.HEADER_HPKP + ? "include-subdomains.pinning.example.com" + : "includesubdomains.preloaded.test"; + let preloadedURI = Services.io.newURI(`https://${preloadedHost}`); + ok(gSSService.isSecureURI(type, preloadedURI, flags)); + gSSService.processHeader( + type, + preloadedURI, + "max-age=1000;" + headerAddendum, + secInfo, + flags, + Ci.nsISiteSecurityService.SOURCE_ORGANIC_REQUEST + ); + ok(gSSService.isSecureURI(type, preloadedURI, flags)); + gSSService.resetState(type, preloadedURI, flags); + ok(gSSService.isSecureURI(type, preloadedURI, flags)); + + // Simulate visiting a preloaded site that unsets HSTS/HPKP by processing an + // HSTS/HPKP header with "max-age=0", check that the HSTS/HPKP bit is what we + // expect (see below), simulate "forget about this site" (call removeState), + // and then check that the HSTS/HPKP bit is set. + gSSService.processHeader( + type, + preloadedURI, + "max-age=0;" + headerAddendum, + secInfo, + flags, + Ci.nsISiteSecurityService.SOURCE_ORGANIC_REQUEST + ); + // Due to architectural constraints, encountering a "max-age=0" header for a + // preloaded HPKP site does not mark that site as not HPKP (whereas with HSTS, + // it does). + if (type == Ci.nsISiteSecurityService.HEADER_HPKP) { + ok(gSSService.isSecureURI(type, preloadedURI, flags)); + } else { + ok(!gSSService.isSecureURI(type, preloadedURI, flags)); + } + gSSService.resetState(type, preloadedURI, flags); + ok(gSSService.isSecureURI(type, preloadedURI, flags)); +} diff --git a/security/manager/ssl/tests/unit/test_sts_fqdn.js b/security/manager/ssl/tests/unit/test_sts_fqdn.js index a3cbad6f727f..78b3cabc86bc 100644 --- a/security/manager/ssl/tests/unit/test_sts_fqdn.js +++ b/security/manager/ssl/tests/unit/test_sts_fqdn.js @@ -29,7 +29,7 @@ function run_test() { ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri1, 0)); ok(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri2, 0)); - SSService.removeState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0); + SSService.resetState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0); ok(!SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0)); ok(!SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri1, 0)); ok(!SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS, uri2, 0)); diff --git a/security/manager/ssl/tests/unit/xpcshell.ini b/security/manager/ssl/tests/unit/xpcshell.ini index a57f2f248042..d28e1bd97050 100644 --- a/security/manager/ssl/tests/unit/xpcshell.ini +++ b/security/manager/ssl/tests/unit/xpcshell.ini @@ -199,6 +199,7 @@ skip-if = toolkit == 'android' [test_sss_readstate_empty.js] [test_sss_readstate_garbage.js] [test_sss_readstate_huge.js] +[test_sss_resetState.js] [test_sss_savestate.js] [test_sss_sanitizeOnShutdown.js] firefox-appdir = browser diff --git a/testing/specialpowers/content/SpecialPowersAPIParent.jsm b/testing/specialpowers/content/SpecialPowersAPIParent.jsm index ec3b03ce7d2c..d11fb4b7d3bd 100644 --- a/testing/specialpowers/content/SpecialPowersAPIParent.jsm +++ b/testing/specialpowers/content/SpecialPowersAPIParent.jsm @@ -705,7 +705,7 @@ class SpecialPowersAPIParent extends JSWindowActorParent { let sss = Cc["@mozilla.org/ssservice;1"].getService( Ci.nsISiteSecurityService ); - sss.removeState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, flags); + sss.resetState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, flags); return undefined; } diff --git a/toolkit/components/cleardata/ClearDataService.jsm b/toolkit/components/cleardata/ClearDataService.jsm index 3e45fae3b0a1..136d06d1716a 100644 --- a/toolkit/components/cleardata/ClearDataService.jsm +++ b/toolkit/components/cleardata/ClearDataService.jsm @@ -920,14 +920,14 @@ const SecuritySettingsCleaner = { Ci.nsISiteSecurityService.HEADER_HSTS, Ci.nsISiteSecurityService.HEADER_HPKP, ]) { - // Also remove HSTS/HPKP/OMS information for subdomains by enumerating + // Also remove HSTS/HPKP information for subdomains by enumerating // the information in the site security service. for (let entry of sss.enumerate(type)) { let hostname = entry.hostname; if (Services.eTLD.hasRootDomain(hostname, aHost)) { - // This uri is used as a key to remove the state. + // This uri is used as a key to reset the state. let uri = Services.io.newURI("https://" + hostname); - sss.removeState(type, uri, 0, entry.originAttributes); + sss.resetState(type, uri, 0, entry.originAttributes); } } } From 9e92bbfdc8502748fd5b0a8f3d10982037ae8358 Mon Sep 17 00:00:00 2001 From: Gurzau Raul Date: Wed, 17 Jul 2019 00:07:39 +0300 Subject: [PATCH 5/5] Backed out 2 changesets (bug 1557096, bug 1565410) for multiple regressions linked to Bug 1557096. a=backout CLOSED TREE Backed out changeset d35d90d0322b (bug 1565410) Backed out changeset 4629e855ea33 (bug 1557096) --HG-- extra : amend_source : a32a416570e06b3905d482ebf313a0da21e53cb6 --- build.gradle | 2 +- mobile/android/geckoview/api.txt | 6 +- .../geckoview/test/ContentDelegateTest.kt | 63 +------------------ .../test/rule/GeckoSessionTestRule.java | 2 +- .../geckoview/test/util/RuntimeCreator.java | 7 ++- .../org/mozilla/geckoview/GeckoRuntime.java | 4 +- .../org/mozilla/geckoview/GeckoSession.java | 20 +----- .../geckoview/WebExtensionController.java | 28 ++++----- .../mozilla/geckoview/doc-files/CHANGELOG.md | 5 +- .../modules/geckoview/ContentCrashHandler.jsm | 2 +- .../modules/geckoview/GeckoViewContent.jsm | 33 ++-------- 11 files changed, 35 insertions(+), 137 deletions(-) diff --git a/build.gradle b/build.gradle index a9961f98ea2f..8b91888b5d7f 100644 --- a/build.gradle +++ b/build.gradle @@ -83,7 +83,7 @@ buildscript { } dependencies { - classpath 'org.mozilla.apilint:apilint:0.2.2' + classpath 'org.mozilla.apilint:apilint:0.2.1' classpath 'com.android.tools.build:gradle:3.1.4' classpath 'com.getkeepsafe.dexcount:dexcount-gradle-plugin:0.8.2' classpath 'org.apache.commons:commons-exec:1.3' diff --git a/mobile/android/geckoview/api.txt b/mobile/android/geckoview/api.txt index e9e43400c881..c1a98bd21479 100644 --- a/mobile/android/geckoview/api.txt +++ b/mobile/android/geckoview/api.txt @@ -56,6 +56,9 @@ import java.util.AbstractSequentialList; import java.util.List; import java.util.Map; import org.json.JSONObject; +import org.mozilla.gecko.util.BundleEventListener; +import org.mozilla.gecko.util.EventCallback; +import org.mozilla.gecko.util.GeckoBundle; import org.mozilla.geckoview.AllowOrDeny; import org.mozilla.geckoview.CompositorController; import org.mozilla.geckoview.ContentBlocking; @@ -475,7 +478,6 @@ package org.mozilla.geckoview { method @UiThread default public void onFirstComposite(@NonNull GeckoSession); method @UiThread default public void onFocusRequest(@NonNull GeckoSession); method @UiThread default public void onFullScreen(@NonNull GeckoSession, boolean); - method @UiThread default public void onKill(@NonNull GeckoSession); method @UiThread default public void onTitleChange(@NonNull GeckoSession, @Nullable String); method @UiThread default public void onWebAppManifest(@NonNull GeckoSession, @NonNull JSONObject); } @@ -1106,7 +1108,7 @@ package org.mozilla.geckoview { method default public void onPortMessage(@NonNull Object, @NonNull WebExtension.Port); } - public class WebExtensionController { + public class WebExtensionController implements BundleEventListener { ctor protected WebExtensionController(GeckoRuntime, WebExtensionEventDispatcher); method @UiThread @Nullable public WebExtensionController.TabDelegate getTabDelegate(); method @UiThread public void setTabDelegate(@Nullable WebExtensionController.TabDelegate); diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt index 54ea86a5a3e3..521acc79559a 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ContentDelegateTest.kt @@ -4,15 +4,12 @@ package org.mozilla.geckoview.test -import android.app.ActivityManager -import android.content.Context import android.graphics.Matrix import android.graphics.SurfaceTexture import android.net.Uri import android.os.Build import android.os.Bundle import android.os.LocaleList -import android.os.Process import org.mozilla.geckoview.AllowOrDeny import org.mozilla.geckoview.GeckoResult import org.mozilla.geckoview.GeckoSession @@ -22,7 +19,6 @@ import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.IgnoreCrash import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.WithDisplay import org.mozilla.geckoview.test.util.Callbacks -import android.support.annotation.AnyThread import android.support.test.filters.MediumTest import android.support.test.filters.SdkSuppress import android.support.test.runner.AndroidJUnit4 @@ -41,7 +37,6 @@ import org.json.JSONObject import org.junit.Assume.assumeThat import org.junit.Test import org.junit.runner.RunWith -import org.mozilla.gecko.GeckoAppShell import org.mozilla.geckoview.test.rule.GeckoSessionTestRule @RunWith(AndroidJUnit4::class) @@ -104,7 +99,7 @@ class ContentDelegateTest : BaseSessionTest() { assertThat("Session should be closed after a crash", session.isOpen, equalTo(false)) } - }) + }); // Recover immediately mainSession.open() @@ -175,62 +170,6 @@ class ContentDelegateTest : BaseSessionTest() { } } - @AnyThread - fun killContentProcess() { - val context = GeckoAppShell.getApplicationContext() - val manager = context.getSystemService(Context.ACTIVITY_SERVICE) as ActivityManager - for (info in manager.runningAppProcesses) { - if (info.processName.endsWith(":tab")) { - Process.killProcess(info.pid) - } - } - } - - @IgnoreCrash - @Test fun killContent() { - assumeThat(sessionRule.env.isMultiprocess, equalTo(true)) - assumeThat(sessionRule.env.isDebugBuild && sessionRule.env.isX86, - equalTo(false)) - - killContentProcess() - mainSession.waitUntilCalled(object : Callbacks.ContentDelegate { - @AssertCalled(count = 1) - override fun onKill(session: GeckoSession) { - assertThat("Session should be closed after being killed", - session.isOpen, equalTo(false)) - } - }) - - mainSession.open() - mainSession.loadTestPath(HELLO_HTML_PATH) - mainSession.waitUntilCalled(object : Callbacks.ProgressDelegate { - @AssertCalled(count = 1) - override fun onPageStop(session: GeckoSession, success: Boolean) { - assertThat("Page should load successfully", success, equalTo(true)) - } - }) - } - - @IgnoreCrash - @Test fun killContentMultipleSessions() { - assumeThat(sessionRule.env.isMultiprocess, equalTo(true)) - assumeThat(sessionRule.env.isDebugBuild && sessionRule.env.isX86, - equalTo(false)) - - val newSession = sessionRule.createOpenSession() - killContentProcess() - - val remainingSessions = mutableListOf(newSession, mainSession) - while (remainingSessions.isNotEmpty()) { - sessionRule.waitUntilCalled(object : Callbacks.ContentDelegate { - @AssertCalled(count = 1) - override fun onKill(session: GeckoSession) { - remainingSessions.remove(session) - } - }) - } - } - // TextInputDelegateTest is parameterized, so we put this test under ContentDelegateTest. @SdkSuppress(minSdkVersion = 23) @Test fun autofill() { diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java index a05e5b18ced9..cfc9c9043bdf 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java @@ -103,7 +103,7 @@ public class GeckoSessionTestRule implements TestRule { sOnNewSession = GeckoSession.NavigationDelegate.class.getMethod( "onNewSession", GeckoSession.class, String.class); sOnCrash = GeckoSession.ContentDelegate.class.getMethod( - "onKill", GeckoSession.class); + "onCrash", GeckoSession.class); } catch (final NoSuchMethodException e) { throw new RuntimeException(e); } diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java index b89c15b8cf63..2f85875786ea 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java @@ -81,8 +81,11 @@ public class RuntimeCreator { runtimeSettingsBuilder.arguments(new String[]{"-purgecaches"}) .extras(InstrumentationRegistry.getArguments()) .remoteDebuggingEnabled(true) - .consoleOutput(true) - .crashHandler(TestCrashHandler.class); + .consoleOutput(true); + + if (new Environment().isAutomation()) { + runtimeSettingsBuilder.crashHandler(TestCrashHandler.class); + } TEST_SUPPORT_WEB_EXTENSION.setMessageDelegate(sMessageDelegate, "browser"); diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java index 276222dd91c7..3ec58c128643 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java @@ -194,7 +194,7 @@ public final class GeckoRuntime implements Parcelable { if ("Gecko:Exited".equals(event) && mDelegate != null) { mDelegate.onShutdown(); EventDispatcher.getInstance().unregisterUiThreadListener(mEventListener, "Gecko:Exited"); - } else if ("GeckoView:ContentCrashReport".equals(event) && crashHandler != null) { + } else if ("GeckoView:ContentCrash".equals(event) && crashHandler != null) { final Context context = GeckoAppShell.getApplicationContext(); Intent i = new Intent(ACTION_CRASHED, null, context, crashHandler); @@ -244,7 +244,7 @@ public final class GeckoRuntime implements Parcelable { throw new IllegalArgumentException("Crash handler service must run in a separate process"); } - EventDispatcher.getInstance().registerUiThreadListener(mEventListener, "GeckoView:ContentCrashReport"); + EventDispatcher.getInstance().registerUiThreadListener(mEventListener, "GeckoView:ContentCrash"); flags |= GeckoThread.FLAG_ENABLE_NATIVE_CRASHREPORTER; } catch (PackageManager.NameNotFoundException e) { diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java index f4522f16061f..7c6dcb3d9d24 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java @@ -440,7 +440,6 @@ public class GeckoSession implements Parcelable { "GeckoViewContent", this, new String[]{ "GeckoView:ContentCrash", - "GeckoView:ContentKill", "GeckoView:ContextMenu", "GeckoView:DOMTitleChanged", "GeckoView:DOMWindowClose", @@ -456,12 +455,10 @@ public class GeckoSession implements Parcelable { final String event, final GeckoBundle message, final EventCallback callback) { + if ("GeckoView:ContentCrash".equals(event)) { close(); delegate.onCrash(GeckoSession.this); - } else if ("GeckoView:ContentKill".equals(event)) { - close(); - delegate.onKill(GeckoSession.this); } else if ("GeckoView:ContextMenu".equals(event)) { final ContentDelegate.ContextElement elem = new ContentDelegate.ContextElement( @@ -3126,24 +3123,11 @@ public class GeckoSession implements Parcelable { * is preserved. Most applications will want to call * {@link #loadUri(Uri)} or {@link #restoreState(SessionState)} at this point. * - * @param session The GeckoSession for which the content process has crashed. + * @param session The GeckoSession that crashed. */ @UiThread default void onCrash(@NonNull GeckoSession session) {} - /** - * The content process hosting this GeckoSession has been killed. The - * GeckoSession is now closed and unusable. You may call - * {@link #open(GeckoRuntime)} to recover the session, but no state - * is preserved. Most applications will want to call - * {@link #loadUri(Uri)} or {@link #restoreState(SessionState)} at this point. - * - * @param session The GeckoSession for which the content process has been killed. - */ - @UiThread - default void onKill(@NonNull GeckoSession session) {} - - /** * Notification that the first content composition has occurred. * This callback is invoked for the first content composite after either diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java index d7fb951045ba..a1775ad604f1 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java @@ -8,7 +8,7 @@ import org.mozilla.gecko.util.BundleEventListener; import org.mozilla.gecko.util.EventCallback; import org.mozilla.gecko.util.GeckoBundle; -public class WebExtensionController { +public class WebExtensionController implements BundleEventListener { public interface TabDelegate { /** * Called when tabs.create is invoked, this method returns a *newly-created* session @@ -28,23 +28,10 @@ public class WebExtensionController { private GeckoRuntime mRuntime; private WebExtensionEventDispatcher mDispatcher; private TabDelegate mTabDelegate; - private final EventListener mEventListener; protected WebExtensionController(final GeckoRuntime runtime, final WebExtensionEventDispatcher dispatcher) { mRuntime = runtime; mDispatcher = dispatcher; - mEventListener = new EventListener(); - } - - private class EventListener implements BundleEventListener { - @Override - public void handleMessage(final String event, final GeckoBundle message, - final EventCallback callback) { - if ("GeckoView:WebExtension:NewTab".equals(event)) { - newTab(message, callback); - return; - } - } } @UiThread @@ -52,14 +39,14 @@ public class WebExtensionController { if (delegate == null) { if (mTabDelegate != null) { EventDispatcher.getInstance().unregisterUiThreadListener( - mEventListener, + this, "GeckoView:WebExtension:NewTab" ); } } else { if (mTabDelegate == null) { EventDispatcher.getInstance().registerUiThreadListener( - mEventListener, + this, "GeckoView:WebExtension:NewTab" ); } @@ -102,4 +89,13 @@ public class WebExtensionController { callback.sendSuccess(session.getId()); }); } + + @Override + public void handleMessage(final String event, final GeckoBundle message, + final EventCallback callback) { + if ("GeckoView:WebExtension:NewTab".equals(event)) { + newTab(message, callback); + return; + } + } } diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md index 3a4cc74ecf45..49a85e620be8 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md @@ -34,9 +34,6 @@ exclude: true [`browser.tabs.create`][69.6] calls by WebExtensions. [69.6]: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/create -[69.7]: ../GeckoSession.ContentDelegate.html#onKill - -- Created `onKill` to `ContentDelegate` to differentiate from crashes. ## v68 - Added [`GeckoRuntime#configurationChanged`][68.1] to notify the device @@ -350,4 +347,4 @@ exclude: true [65.24]: ../CrashReporter.html#sendCrashReport-android.content.Context-android.os.Bundle-java.lang.String- [65.25]: ../GeckoResult.html -[api-version]: a48d987bbc9301577c072823070ff5541e0fa211 +[api-version]: 3d726407275d906a54c1fc86eb92f2c3bfaaa3d0 diff --git a/mobile/android/modules/geckoview/ContentCrashHandler.jsm b/mobile/android/modules/geckoview/ContentCrashHandler.jsm index 50839bd28c98..6eb2db3a6c79 100644 --- a/mobile/android/modules/geckoview/ContentCrashHandler.jsm +++ b/mobile/android/modules/geckoview/ContentCrashHandler.jsm @@ -65,7 +65,7 @@ var ContentCrashHandler = { const [minidumpPath, extrasPath] = getPendingMinidump(dumpID); EventDispatcher.instance.sendRequest({ - type: "GeckoView:ContentCrashReport", + type: "GeckoView:ContentCrash", minidumpPath, extrasPath, success: true, diff --git a/mobile/android/modules/geckoview/GeckoViewContent.jsm b/mobile/android/modules/geckoview/GeckoViewContent.jsm index d2099b53910a..f853ff8b0be7 100644 --- a/mobile/android/modules/geckoview/GeckoViewContent.jsm +++ b/mobile/android/modules/geckoview/GeckoViewContent.jsm @@ -60,7 +60,6 @@ class GeckoViewContent extends GeckoViewModule { ); Services.obs.addObserver(this, "oop-frameloader-crashed"); - Services.obs.addObserver(this, "ipc:content-shutdown"); } onDisable() { @@ -90,7 +89,6 @@ class GeckoViewContent extends GeckoViewModule { ); Services.obs.removeObserver(this, "oop-frameloader-crashed"); - Services.obs.removeObserver(this, "ipc:content-shutdown"); } // Bundle event handler. @@ -197,38 +195,17 @@ class GeckoViewContent extends GeckoViewModule { // nsIObserver event handler observe(aSubject, aTopic, aData) { debug`observe: ${aTopic}`; - this._contentCrashed = false; - const browser = aSubject.ownerElement; switch (aTopic) { case "oop-frameloader-crashed": { + const browser = aSubject.ownerElement; if (!browser || browser != this.browser) { return; } - this.window.setTimeout(() => { - if (this._contentCrashed) { - this.eventDispatcher.sendRequest({ - type: "GeckoView:ContentCrash", - }); - } else { - this.eventDispatcher.sendRequest({ - type: "GeckoView:ContentKill", - }); - } - }, 250); - break; - } - case "ipc:content-shutdown": { - aSubject.QueryInterface(Ci.nsIPropertyBag2); - if (aSubject.get("dumpID")) { - if ( - browser && - aSubject.get("childID") != browser.frameLoader.childID - ) { - return; - } - this._contentCrashed = true; - } + + this.eventDispatcher.sendRequest({ + type: "GeckoView:ContentCrash", + }); break; } }