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

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Jorg K (CEST = GMT+2)]

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

and:

=== 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.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1341060-null-docshell.patch (v1).

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

Boris, could you please review this.

Comment 3

[Boris Zbarsky [: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 "&&"?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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

[Jorg K (CEST = GMT+2)]

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

OK, let's get Magnus to review it then.

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e37dc49884ddf30eda2aa67cb947fb3e5922ff70