1301399 - ASSERTION: no SHEntry for a non-transient viewer?: 'NS_IsAboutBlank(mCurrentURI)

Status: RESOLVED FIXED

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I can see this assertion when running the attachment 8693720 [details] via marionette in a debug build on Linux64: "mach marionette-test %test_file%".

Current line of failure:
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11521

I tried to get some nsDocShell log output ("MOZLOG=nsDocShell:5") but no output is shown. Maybe its a problem with the debug artifact build?

Full stacktrace see attachment 8787164 [details]:
#01: nsDocShell::OnLoadingSite(nsIChannel*, bool, bool) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#02: nsDocShell::CreateContentViewer(nsACString_internal const&, nsIRequest*, nsIStreamListener**) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#03: nsDSURIContentListener::DoContent(nsACString_internal const&, bool, nsIRequest*, nsIStreamListener**, bool*) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#04: nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#05: nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#06: nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#07: mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)
#08: mozilla::net::HttpChannelChild::OnStartRequest(nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, unsigned int const&) (/mozilla/code/gecko/obj/debug/dist/bin/libxul.so)

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

It'll be a bit before I have a chance to look into this. It would be a huge help if |mach marionette-test| took a --debugger option to run firefox under, as far as I can tell, it currently only allows attaching either a Python or JS debugger.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I just checked and indeed it's not available. I found bug 1261180 which actually tracks its addition.

Comment 3

[Samael Wang [:freesamael] (away for now)]

The assert is also hit by history navigation between content / chrome URLs. An example is

1. Open Firefox, which should load about:home in content process.
2. Type about:config to force it load the URL in chrome process instead.
3. Press back. Firefox would use session store to restore the history in content process, and you'll see the assertion in log.

My understanding for this specific case is because ContentRestore.jsm changes currentURI [1]. An easy fix for this specific case is to pass isRemotenessUpdate when calling the function [2], and just don't set URI if isRemotenessUpdate.

But I think the more important issue is that we provided setCurrentURI as a public API nsIDocShell, which makes mCurrentURI not a reliable source to check loaded document. According to the comment few lines above [1] you can see people were trying to avoid this assertion, but then forgot to update it when the arguments were changed. I think we'll keep hitting this assertion if we keep mCurrentURI unreliable. Should we modify docshell to avoid the situation? Like testing document URI instead (not sure if it's more reliable)?

[1] http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/browser/components/sessionstore/ContentRestore.jsm#133
[2] http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/browser/components/sessionstore/content/content-sessionStore.js#171

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: minimized testcase 1

First minimized testcase to reproduce this issue. Just run via "mach marionette-test path_to_file".

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: minimized testcase 2

Second minimized testcase to reproduce this problem. Run it with the same command as the first testcase.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The current behavior actually causes Marionette to hang when navigating through pages, and finally quit with an exit code -15. The problem is covered by bug 1305659.

It would be great if we could get this fixed in case it is easy to do.

Comment 7

[Samael Wang [:freesamael] (away for now)]

Attachment: Fix-content-restore-to-prevent-docshell-assertion.patch

(In reply to Henrik Skupin (:whimboo) from comment #6)
> The current behavior actually causes Marionette to hang when navigating
> through pages, and finally quit with an exit code -15. The problem is
> covered by bug 1305659.
> 
> It would be great if we could get this fixed in case it is easy to do.

I tried the test case with this patch, the log shows no assertion but the test case still hang. I guess it's not the root cause.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I'm not likely to get to this in the near future.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

:qdot - this is coming up a bunch with our process switching code right now. Any chance you could spare a couple of cycles to see if we can just silence or fix this assertion?

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Welp. I just hit this in the frameloader process switching bug, bug 1522713. So I guess I'll take a look at it now.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

I'm guessing we should probably get rid of this assertion if we're going to continue hitting it due to sessionstore, especially when switching processes. :peterv, if you've seen this code before, do you know if that would be a bad idea?
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/docshell/base/nsDocShell.cpp#10938-10945

Comment 13

[Peter Van der Beken [:peterv]]

I don't know what this assertion was trying to catch. The fact that we fake mCurrentURI for the url bar seems a bit nuts too though.

Comment 15

[Paul Bone [:pbone]]

Attachment: Bug 1301399 - Remove annoying session history assertion r=nika

Comment 16

[Paul Bone [:pbone]]

Comment on attachment 8815592 [details] [diff] [review]
Fix-content-restore-to-prevent-docshell-assertion.patch

Session history will be re-worked for fission so this likely isn't relevant.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4df52de803ad
Remove annoying session history assertion r=nika

Comment 18

[Paul Bone [:pbone]]

Attachment: Bug 1301399 - Fix comment after removing code r=nika

Depends on D60773

Comment 19

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/4df52de803ad

Comment 20

[Paul Bone [:pbone]]

Reopened because there's a follow up patch I want to land (it should have been in the first patch).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac1369329352
Fix comment after removing code r=nika

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/866a2ed32b60
Delete trailing whitespace. r=pedantic-fix

Comment 23

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ac1369329352
https://hg.mozilla.org/mozilla-central/rev/866a2ed32b60