Bug 1432396 - Add assertions that we don't allow reinitialization of variables that were reset in nsDocShell::Destroy(). r?bz draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Fri, 16 Feb 2018 06:29:59 +0900
changeset 755928 b10b6eaad50626fd5c5aaeddb5df443803a038b5
parent 755927 1b8033cc9c3e2e32b3c1d0cafef28a56edd7cb84
push id99325
push userhikezoe@mozilla.com
push dateThu, 15 Feb 2018 23:56:43 +0000
reviewersbz
bugs1432396
milestone60.0a1
Bug 1432396 - Add assertions that we don't allow reinitialization of variables that were reset in nsDocShell::Destroy(). r?bz Here is the list of variables which are reset in nsDocShell::Destroy(). The right column shows the function where the variable is set. In this patch all the functions other than SetTreeOwner(*1) have an assertion that we don't allow the function to be called or bail out during destroying the docshell. It is possible that SetTreeOwner() is called in script through `browser.setAttribute("primary", "true")`. So we can't assert there, we just bail out from SetTreeOwner when the docshell is being destroyed. mInitialClientSource MaybeCreateInitialClientSource() mObserveErrorPages: Initially true, never sets true again mLoadingURI: CreateContentViewer() mOSHE->SetEditorData(nullptr): DetachEditorFromWindow() mLSHE->SetEditorData(nullptr): Setting again in RestoreFromHistory() after bailing out if mIsBeingDestroyed is true mContentListener: Init() Stop(nsIWebNavigation::STOP_ALL) mRestorePresentationEvent: RestorePresentation() mFailedChannel: LoadErrorPage() mFailedURI: LoadErrorPage() mRefreshURIList: RefreshURI() mEditorData: ReattachEditorToWindow() and EnsureEditorData() mTransferableHookData: EnsureTransferableHookData() mContentViewer: SetupNewViewer() mParentWidget: SetParentWidget() mCurrentURI: SetCurrentURI() mScriptGlobal: EnsureScriptEnvironment() mSessionHistory SetSessionHistory() SetTreeOwner(): SetTreeOwner() *1 mOnePermittedSandboxedNavigator: SetOnePermittedSandboxedNavigator() mSecurityUI: SetSecurityUI() CancelRefreshURITimers() mRefreshURIList: RefreshURI() mSavedRefreshURIList: RefreshURI() mPrivateBrowsingId: SetPrivateBrowsing() mOriginAttributes: SetOriginAttributes() DecreasePrivateDocShellCount: SetAffectPrivateSessionLifetime() MozReview-Commit-ID: G5d941R9K8V
docshell/base/nsDocShell.cpp
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -447,16 +447,18 @@ nsDocShell::~nsDocShell()
                   nsIDToCString(mHistoryID).get());
   }
 #endif
 }
 
 nsresult
 nsDocShell::Init()
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   nsresult rv = nsDocLoader::Init();
   NS_ENSURE_SUCCESS(rv, rv);
 
   NS_ASSERTION(mLoadGroup, "Something went wrong!");
 
   mContentListener = new nsDSURIContentListener(this);
   rv = mContentListener->Init();
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1421,16 +1423,18 @@ nsDocShell::SetCurrentURI(nsIURI* aURI)
   SetCurrentURI(aURI, nullptr, true, 0);
   return NS_OK;
 }
 
 bool
 nsDocShell::SetCurrentURI(nsIURI* aURI, nsIRequest* aRequest,
                           bool aFireOnLocationChange, uint32_t aLocationFlags)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   MOZ_LOG(gDocShellLeakLog, LogLevel::Debug,
           ("DOCSHELL %p SetCurrentURI %s\n",
            this, aURI ? aURI->GetSpecOrDefault().get() : ""));
 
   // We don't want to send a location change when we're displaying an error
   // page, and we don't want to change our idea of "current URI" either
   if (mLoadType == LOAD_ERROR_PAGE) {
     return false;
@@ -1748,16 +1752,18 @@ nsDocShell::SetUsePrivateBrowsing(bool a
   }
 
   return SetPrivateBrowsing(aUsePrivateBrowsing);
 }
 
 NS_IMETHODIMP
 nsDocShell::SetPrivateBrowsing(bool aUsePrivateBrowsing)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   bool changed = aUsePrivateBrowsing != (mPrivateBrowsingId > 0);
   if (changed) {
     mPrivateBrowsingId = aUsePrivateBrowsing ? 1 : 0;
 
     if (mItemType != typeChrome) {
       mOriginAttributes.SyncAttributesWithPrivateBrowsing(aUsePrivateBrowsing);
     }
 
@@ -1823,16 +1829,18 @@ nsDocShell::SetRemoteTabs(bool aUseRemot
 
   mUseRemoteTabs = aUseRemoteTabs;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDocShell::SetAffectPrivateSessionLifetime(bool aAffectLifetime)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   bool change = aAffectLifetime != mAffectPrivateSessionLifetime;
   if (change && UsePrivateBrowsing()) {
     AssertOriginAttributesMatchPrivateBrowsing();
     if (aAffectLifetime) {
       IncreasePrivateDocShellCount();
     } else {
       DecreasePrivateDocShellCount();
     }
@@ -2325,16 +2333,18 @@ nsDocShell::GetSecurityUI(nsISecureBrows
 {
   NS_IF_ADDREF(*aSecurityUI = mSecurityUI);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDocShell::SetSecurityUI(nsISecureBrowserUI* aSecurityUI)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   mSecurityUI = aSecurityUI;
   mSecurityUI->SetDocShell(this);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDocShell::GetLoadURIDelegate(nsILoadURIDelegate** aLoadURIDelegate)
 {
@@ -2783,16 +2793,18 @@ nsDocShell::GetParentDocshell()
 {
   nsCOMPtr<nsIDocShell> docshell = do_QueryInterface(GetAsSupports(mParent));
   return docshell.forget().downcast<nsDocShell>();
 }
 
 void
 nsDocShell::MaybeCreateInitialClientSource(nsIPrincipal* aPrincipal)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   // If there is an existing document then there is no need to create
   // a client for a future initial about:blank document.
   if (mScriptGlobal && mScriptGlobal->GetCurrentInnerWindowInternal() &&
       mScriptGlobal->GetCurrentInnerWindowInternal()->GetExtantDoc()) {
     MOZ_DIAGNOSTIC_ASSERT(
       mScriptGlobal->GetCurrentInnerWindowInternal()->GetClientInfo().isSome());
     MOZ_DIAGNOSTIC_ASSERT(!mInitialClientSource);
     return;
@@ -3582,16 +3594,20 @@ PrintDocTree(nsIDocShellTreeItem* aParen
 
   PrintDocTree(parentItem, 0);
 }
 #endif
 
 NS_IMETHODIMP
 nsDocShell::SetTreeOwner(nsIDocShellTreeOwner* aTreeOwner)
 {
+  if (mIsBeingDestroyed && aTreeOwner) {
+    return NS_ERROR_FAILURE;
+  }
+
 #ifdef DEBUG_DOCSHELL_FOCUS
   nsCOMPtr<nsIDocShellTreeItem> item(do_QueryInterface(aTreeOwner));
   if (item) {
     PrintDocTree(item);
   }
 #endif
 
   // Don't automatically set the progress based on the tree owner for frames
@@ -4927,16 +4943,18 @@ nsDocShell::DisplayLoadError(nsresult aE
 nsresult
 nsDocShell::LoadErrorPage(nsIURI* aURI, const char16_t* aURL,
                           const char* aErrorPage,
                           const char* aErrorType,
                           const char16_t* aDescription,
                           const char* aCSSClass,
                           nsIChannel* aFailedChannel)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
 #if defined(DEBUG)
   if (MOZ_LOG_TEST(gDocShellLog, LogLevel::Debug)) {
     nsAutoCString chanName;
     if (aFailedChannel) {
       aFailedChannel->GetName(chanName);
     } else {
       chanName.AssignLiteral("<no channel>");
     }
@@ -5237,16 +5255,18 @@ nsDocShell::GetReferringURI(nsIURI** aUR
 
 NS_IMETHODIMP
 nsDocShell::SetSessionHistory(nsISHistory* aSessionHistory)
 {
   NS_ENSURE_TRUE(aSessionHistory, NS_ERROR_FAILURE);
   // make sure that we are the root docshell and
   // set a handle to root docshell in SH.
 
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   nsCOMPtr<nsIDocShellTreeItem> root;
   /* Get the root docshell. If *this* is the root docshell
    * then save a handle to *this* in SH. SH needs it to do
    * traversions thro' its entries
    */
   GetSameTypeRootTreeItem(getter_AddRefs(root));
   NS_ENSURE_TRUE(root, NS_ERROR_FAILURE);
   if (root.get() == static_cast<nsIDocShellTreeItem*>(this)) {
@@ -5418,16 +5438,20 @@ nsDocShell::Create()
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDocShell::Destroy()
 {
+  // XXX: We allow this function to be called just once.  If you are going to
+  // reset new variables in this function, please make sure the variables will
+  // never be re-initialized.  Adding assertions to check |mIsBeingDestroyed|
+  // in the setter functions for the variables would be enough.
   if (mIsBeingDestroyed) {
     return NS_ERROR_DOCSHELL_DYING;
   }
 
   NS_ASSERTION(mItemType == typeContent || mItemType == typeChrome,
                "Unexpected item type in docshell");
 
   AssertOriginAttributesMatchPrivateBrowsing();
@@ -5713,16 +5737,17 @@ nsDocShell::GetParentWidget(nsIWidget** 
   NS_IF_ADDREF(*aParentWidget);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDocShell::SetParentWidget(nsIWidget* aParentWidget)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
   mParentWidget = aParentWidget;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDocShell::GetParentNativeWindow(nativeWindow* aParentNativeWindow)
 {
@@ -5961,16 +5986,18 @@ nsDocShell::GetSandboxFlags(uint32_t* aS
 NS_IMETHODIMP
 nsDocShell::SetOnePermittedSandboxedNavigator(nsIDocShell* aSandboxedNavigator)
 {
   if (mOnePermittedSandboxedNavigator) {
     NS_ERROR("One Permitted Sandboxed Navigator should only be set once.");
     return NS_OK;
   }
 
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   mOnePermittedSandboxedNavigator = do_GetWeakReference(aSandboxedNavigator);
   NS_ASSERTION(mOnePermittedSandboxedNavigator,
                "One Permitted Sandboxed Navigator must support weak references.");
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -6312,16 +6339,18 @@ nsDocShell::ScrollByPages(int32_t aNumPa
 // nsDocShell::nsIRefreshURI
 //*****************************************************************************
 
 NS_IMETHODIMP
 nsDocShell::RefreshURI(nsIURI* aURI, nsIPrincipal* aPrincipal,
                        int32_t aDelay, bool aRepeat,
                        bool aMetaRefresh)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   NS_ENSURE_ARG(aURI);
 
   /* Check if Meta refresh/redirects are permitted. Some
    * embedded applications may not want to do this.
    * Must do this before sending out NOTIFY_REFRESH events
    * because listeners may have side effects (e.g. displaying a
    * button to manually trigger the refresh later).
    */
@@ -7860,16 +7889,18 @@ nsDocShell::CanSavePresentation(uint32_t
   // If the document does not want its presentation cached, then don't.
   nsCOMPtr<nsIDocument> doc = mScriptGlobal->GetExtantDoc();
   return doc && doc->CanSavePresentation(aNewRequest);
 }
 
 void
 nsDocShell::ReattachEditorToWindow(nsISHEntry* aSHEntry)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   NS_ASSERTION(!mEditorData,
                "Why reattach an editor when we already have one?");
   NS_ASSERTION(aSHEntry && aSHEntry->HasDetachedEditor(),
                "Reattaching when there's not a detached editor.");
 
   if (mEditorData || !aSHEntry) {
     return;
   }
@@ -7897,16 +7928,19 @@ nsDocShell::DetachEditorFromWindow()
                "Detaching editor when it's already detached.");
 
   nsresult res = mEditorData->DetachFromWindow();
   NS_ASSERTION(NS_SUCCEEDED(res), "Failed to detach editor");
 
   if (NS_SUCCEEDED(res)) {
     // Make mOSHE hold the owning ref to the editor data.
     if (mOSHE) {
+      MOZ_ASSERT(!mIsBeingDestroyed || !mOSHE->HasDetachedEditor(),
+                 "We should not set the editor data again once after we "
+                 "detached the editor data during destroying this docshell");
       mOSHE->SetEditorData(mEditorData.forget());
     } else {
       mEditorData = nullptr;
     }
   }
 
 #ifdef DEBUG
   {
@@ -8081,16 +8115,18 @@ nsDocShell::GetRestoringDocument(bool* a
 {
   *aRestoring = mIsRestoringDocument;
   return NS_OK;
 }
 
 nsresult
 nsDocShell::RestorePresentation(nsISHEntry* aSHEntry, bool* aRestoring)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   NS_ASSERTION(mLoadType & LOAD_CMD_HISTORY,
                "RestorePresentation should only be called for history loads");
 
   nsCOMPtr<nsIContentViewer> viewer;
   aSHEntry->GetContentViewer(getter_AddRefs(viewer));
 
 #ifdef DEBUG_PAGE_CACHE
   nsCOMPtr<nsIURI> uri;
@@ -8938,16 +8974,18 @@ nsDocShell::NewContentViewerObj(const ns
 
   (*aViewer)->SetContainer(this);
   return NS_OK;
 }
 
 nsresult
 nsDocShell::SetupNewViewer(nsIContentViewer* aNewViewer)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   //
   // Copy content viewer state from previous or parent content viewer.
   //
   // The following logic is mirrored in nsHTMLDocument::StartDocumentLoad!
   //
   // Do NOT to maintain a reference to the old content viewer outside
   // of this "copying" block, or it will not be destroyed until the end of
   // this routine and all <SCRIPT>s and event handlers fail! (bug 20315)
@@ -12938,31 +12976,35 @@ nsDocShell::EnsureScriptEnvironment()
 
   // Ensure the script object is set up to run script.
   return mScriptGlobal->EnsureScriptEnvironment();
 }
 
 nsresult
 nsDocShell::EnsureEditorData()
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   bool openDocHasDetachedEditor = mOSHE && mOSHE->HasDetachedEditor();
   if (!mEditorData && !mIsBeingDestroyed && !openDocHasDetachedEditor) {
     // We shouldn't recreate the editor data if it already exists, or
     // we're shutting down, or we already have a detached editor data
     // stored in the session history. We should only have one editordata
     // per docshell.
     mEditorData = new nsDocShellEditorData(this);
   }
 
   return mEditorData ? NS_OK : NS_ERROR_NOT_AVAILABLE;
 }
 
 nsresult
 nsDocShell::EnsureTransferableHookData()
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   if (!mTransferableHookData) {
     mTransferableHookData = new nsTransferableHookData();
   }
 
   return NS_OK;
 }
 
 nsresult
@@ -14079,16 +14121,18 @@ nsDocShell::ServiceWorkerAllowedToContro
     nsContentUtils::StorageAllowedForNewWindow(aPrincipal, aURI, parentInner);
 
   return storage == nsContentUtils::StorageAccess::eAllow;
 }
 
 nsresult
 nsDocShell::SetOriginAttributes(const OriginAttributes& aAttrs)
 {
+  MOZ_ASSERT(!mIsBeingDestroyed);
+
   if (!CanSetOriginAttributes()) {
     return NS_ERROR_FAILURE;
   }
 
   AssertOriginAttributesMatchPrivateBrowsing();
   mOriginAttributes = aAttrs;
 
   bool isPrivate = mOriginAttributes.mPrivateBrowsingId > 0;