Bug 1432396 - Add assertions that we don't allow reinitialization of variables that were reset in nsDocShell::Destroy(). r=bz
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Fri, 16 Feb 2018 06:29:59 +0900
changeset 404061 57abd8e4ab25bc21d9c75dbf43546e7272fddf0e
parent 404060 f0f5a81b193f8c8851b0a6448e86c03457966c1a
child 404062 5dc5ecd683c8475459f911a7250e58321e1a2733
push id33450
push usernbeleuzu@mozilla.com
push dateFri, 16 Feb 2018 09:48:23 +0000
treeherdermozilla-central@6ba349d419dd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1432396
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -4931,16 +4947,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>");
     }
@@ -5241,16 +5259,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)) {
@@ -5422,16 +5442,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();
@@ -5717,16 +5741,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)
 {
@@ -5965,16 +5990,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
@@ -6316,16 +6343,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).
    */
@@ -7865,16 +7894,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;
   }
@@ -7902,16 +7933,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
   {
@@ -8086,16 +8120,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;
@@ -8943,16 +8979,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)
@@ -12943,31 +12981,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
@@ -14084,16 +14126,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;