Investigate nsMsgContentPolicy::ShouldLoad() calls where NS_CP_GetDocShellFromContext(aRequestingContext) returns null

RESOLVED FIXED in Thunderbird 54.0


MailNews Core
6 months ago
6 months ago


(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))


Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



6 months ago
Seen in a debug build:

[5800] WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80070057: file c:/mozilla-source/comm-central/mailnews/base/src/nsMsgContentPolicy.cpp, line 660

Which reads:

  nsIDocShell *shell = NS_CP_GetDocShellFromContext(aRequestingContext);
  nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem(do_QueryInterface(shell, &rv));
  NS_ENSURE_SUCCESS(rv, nullptr); <=== Line 660


=== nsMsgContentPolicy::GetOriginatingURIForContext context=0000000012246F58 principle=0000000013638F60
=== nsMsgContentPolicy::GetOriginatingURIForContext context=0000000012246F58
=== nsMsgContentPolicy::GetOriginatingURIForContext shell=0000000000000000
[5800] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:/mozilla-source/comm-central/mailnews/base/src/nsMsgContentPolicy.cpp, line 793

which reads:

  printf("=== nsMsgContentPolicy::GetOriginatingURIForContext context=%p\n", (void*)aRequestingContext);
  nsIDocShell *shell = NS_CP_GetDocShellFromContext(aRequestingContext);
  printf("=== nsMsgContentPolicy::GetOriginatingURIForContext shell=%p\n", (void*)shell);
  nsCOMPtr<nsIDocShellTreeItem> docshellTreeItem(do_QueryInterface(shell, &rv));
  NS_ENSURE_SUCCESS(rv, rv); <=== Line 793
(I added debug to catch this).

So in both cases, NS_CP_GetDocShellFromContext() doesn't return a docshell.

This seems to have been subject of bug 376460 which never went anywhere.

What I observe in a debug session is that suddenly those messages start, I get a few and then they stop. I think bug 376460 comment #6 gives a hint here:
For instance I know that RSS feeds can load without docshells in the background

So maybe we should just ignore the case where a ShouldLoad() call doesn't produce a docshell.


6 months ago
Duplicate of this bug: 376460

Comment 2

6 months ago
Created attachment 8839191 [details] [diff] [review]
1341060-null-docshell.patch (v1).

This should shut up the endless complaints on the debug console.

Boris, could you please review this.
Assignee: nobody → jorgk
Flags: needinfo?(bzbarsky)
Comment on attachment 8839191 [details] [diff] [review]
1341060-null-docshell.patch (v1).

>+    if (NS_FAILED(rv) && !originatorLocation)

That doesn't make any sense.  Did you mean "||" instead of "&&"?
Flags: needinfo?(bzbarsky)

Comment 4

6 months ago
Created attachment 8839196 [details] [diff] [review]
1341060-null-docshell.patch (v2).

Actually, I need to communicate that case back, so perhaps better like this, although GetOriginatingURIForContext() didn't strictly succeed. Do you have a better idea?
Attachment #8839191 - Attachment is obsolete: true
That looks fine to me, generally speaking, and I have no idea at all.  I don't know what this code expects to be happening.

Comment 6

6 months ago
But you're the docshell man ;-) I'm surprised that ShouldLoad() is called with a context and a principle (see debug output in comment #0), yet you can't get a docshell for that. I'd just like to handle the case gracefully instead of dumping a load or errors to the console and then ignoring them anyway. This patch shouldn't change the behaviour at all, should only shut up the errors.
> But you're the docshell man

Yes, but this is mailnews code.

> yet you can't get a docshell for that

I can think of all sorts of ways this can happen in theory (e.g. XHR in a non-system sandbox).  Why it happens in your case, I can't tell you.

Comment 8

6 months ago
Comment on attachment 8839196 [details] [diff] [review]
1341060-null-docshell.patch (v2).

OK, let's get Magnus to review it then.
Attachment #8839196 - Flags: review?(mkmelin+mozilla)

Comment 9

6 months ago
Comment on attachment 8839196 [details] [diff] [review]
1341060-null-docshell.patch (v2).

Review of attachment 8839196 [details] [diff] [review]:

Can't claim to have some deeper understanding of this, but rs=mkmelin
Attachment #8839196 - Flags: review?(mkmelin+mozilla) → review+

Comment 10

6 months ago
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
You need to log in before you can comment on or make changes to this bug.