Navigations from about:tabcrashed add a history entry when SHIP is enabled
Categories
(Core :: DOM: Navigation, defect, P1)
Tracking
()
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
- Open example.com, and ensure it's the active tab.
- Kill the process that example.com lives in (note the pid from the tab and
kill -9
that pid). - Navigate from the about:tabcrashed page to example.org.
- 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
).
Assignee | ||
Comment 3•4 years ago
|
||
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;
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 5•4 years ago
|
||
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.
Assignee | ||
Comment 6•4 years ago
|
||
The test wasn't working without fission, and I needed to debug why.
Assignee | ||
Comment 7•4 years ago
|
||
Still need to debug one more failing test: https://treeherder.mozilla.org/jobs?repo=try&revision=c0a7e3429b9be5b177ccc77be2187f867ba1a377&selectedTaskRun=XmD6tIN6RzmIRwVN-gaGtg.0
Updated•4 years ago
|
Comment 9•4 years ago
|
||
bugherder |
Description
•