Closed Bug 1674464 Opened 4 years ago Closed 4 years ago

Navigations from about:tabcrashed add a history entry when SHIP is enabled

Categories

(Core :: DOM: Navigation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
87 Branch
Fission Milestone M6c
Tracking Status
firefox87 --- fixed

People

(Reporter: u608768, Assigned: peterv)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: fission-ship)

Attachments

(1 file)

STR

  1. Open example.com, and ensure it's the active tab.
  2. Kill the process that example.com lives in (note the pid from the tab and kill -9 that pid).
  3. Navigate from the about:tabcrashed page to example.org.
  4. Click back.

You'll notice that clicking back takes you to about:tabcrashed. With Fission disabled, it goes to example.com instead.

This is causing browser_crashedTabs.js to fail (might need some of the changes from bug 1673617). I'm fairly sure this is a sessionstore bug, but it could be session history as well.

(In reply to :kashav from comment #0)

I'm fairly sure this is a sessionstore bug, but it could be session history as well.

Looks like we never even add an about:tabcrashed nsISHEntry for non-SHIP. This is probably a SessionHistory bug then. We add the entry in nsDocShell::MoveLoadingToActiveEntry, which is called here.

From peterv on Matrix:

LoadErrorPage makes us hold on to the loading entry if it's called during a load
in nsDocShell::MoveLoadingToActiveEntry we then want to add that entry with the original load type
but if we don't have a loading entry in LoadErrorPage then it's called for a crash after the load has finished
in that case we should use LOAD_ERROR_PAGE
to avoid another entry to be added for the error page

The following fixes browser_crashedTabs.js

diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -3980,7 +3980,7 @@ nsresult nsDocShell::LoadErrorPage(nsIUR
                                    nsIChannel* aFailedChannel) {
   mFailedChannel = aFailedChannel;
   mFailedURI = aFailedURI;
-  mFailedLoadType = mLoadType;
+  mFailedLoadType = mLoadingEntry ? mLoadType : LOAD_ERROR_PAGE;
 
   if (mLSHE) {
     // Abandon mLSHE's BFCache entry and create a new one.  This way, if

but may cause problems when we revert mLoadType to mFailedToLoadType in CreateContentViewer (specifically in OnNewURI, since mLoadType would now be LOAD_ERROR_PAGE).

Another possible solution would be to have an extra boolean flag (mFailedWithLoadingEntry?) that is normally false, but is set to true in this block: https://searchfox.org/mozilla-central/rev/94d18932991e45ef7ec1d606ccf2e9b37d452f2a/docshell/base/nsDocShell.cpp#4000-4005 (and false in an else block). And then https://searchfox.org/mozilla-central/rev/94d18932991e45ef7ec1d606ccf2e9b37d452f2a/docshell/base/nsDocShell.cpp#13264-13265 would become

    uint32_t loadType =
        mLoadType == LOAD_ERROR_PAGE && mFailedWithLoadingEntry ? mFailedLoadType : mLoadType;
Assignee: nobody → rjesup
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee: rjesup → peterv
Attachment #9186598 - Attachment description: Bug 1674464 - Stop adding entries for about:tabcrashed when SHIP is enabled. → Bug 1674464 - Stop adding entries for about:tabcrashed when SHIP is enabled. r?smaug!

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:peterv, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(peterv)

The test wasn't working without fission, and I needed to debug why.

Flags: needinfo?(peterv)
Blocks: 1679856
Whiteboard: fission-ship
Pushed by pvanderbeken@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5ee8194d2de8 Stop adding entries for about:tabcrashed when SHIP is enabled. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Regressions: 1918167
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: