1446587 - Port Bug 1439713 - Change nsIContentPolicy shouldLoad to take an nsILoadInfo instead of the various arguments

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

It'll use a loadinfo instead of a bunch of separate arguments.  Certainly nsMsgContentPolicy will be affected.  Not sure whether there are any other content policy impls or calls in Thunderbird.

See bug 1439713.

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks Boris, I'm watching bug 1439713. Looks like reviews are still in progress.

Comment 2

[Jorg K (CEST = GMT+2)]

nsMsgContentPolicy::ShouldLoad():
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mailnews/base/src/nsMsgContentPolicy.cpp#141
which will need to change.

Looks like nsMixedContentBlocker.cpp has a good example:
uint32_t aContentType = aLoadInfo->GetExternalContentPolicyType();
nsCOMPtr<nsISupports> aRequestingContext
    = aContentType == nsIContentPolicy::TYPE_DOCUMENT
    ? aLoadInfo->ContextForTopLevelLoad() : aLoadInfo->LoadingNode();
nsCOMPtr<nsIPrincipal> aRequestPrincipal = aLoadInfo->TriggeringPrincipal();
nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadInfo->LoadingPrincipal();
nsCOMPtr<nsIURI> aRequestingLocation;
if (loadingPrincipal) {
  loadingPrincipal->GetURI(getter_AddRefs(aRequestingLocation));
}

nsMsgContentPolicy::ShouldProcess() does pretty much nothing, so that's easy to fix.

===

However, we also call nsIContentPolicy.shouldLoad() in two source files:
https://dxr.mozilla.org/comm-central/search?q=shouldLoad&redirect=false

Christoph, is there a JS example how to change this call in your patch set? I saw a C++ example in nsMixedContentBlocker.cpp. Maybe a bit tricky putting the LoadInfo together in JS.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jorg K (GMT+1) from comment #2)
> Christoph, is there a JS example how to change this call in your patch set?
> I saw a C++ example in nsMixedContentBlocker.cpp. Maybe a bit tricky putting
> the LoadInfo together in JS.

Yeah, it's a little tricky, but what you can do is the following:

let tmpChannel = NetUtil.newChannel({
  uri: uri,
  loadingNode: targetDoc.documentURIObject,
  securityFlags: Ci.nsILoadInfo.SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK,
  contentPolicyType: Ci.nsIContentPolicy.TYPE_IMAGE
});

let tmpLoadInfo = tmpChannel.loadInfo;

contentPolicy.shouldLoad(uri, tmpLoadInfo);

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1446587-shouldLoad.patch

Something like this then. Can the test re-use the load info?

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8960327 [details] [diff] [review]
1446587-shouldLoad.patch

Review of attachment 8960327 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/test/unit/test_nsIMsgContentPolicy.js
@@ +32,5 @@
> +    loadingNode: req_uri,
> +    securityFlags: Ci.nsILoadInfo.SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK,
> +    contentPolicyType: Ci.nsIContentPolicy.TYPE_IMAGE
> +  });
> +  let tmpLoadInfo = tmpChannel.loadInfo;

Unused. Use or remove.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8960327 [details] [diff] [review]
1446587-shouldLoad.patch

Review of attachment 8960327 [details] [diff] [review]:
-----------------------------------------------------------------

I think that looks good; and yes, the testcase can reuse the loadInfo.

::: mailnews/base/test/unit/test_nsIMsgContentPolicy.js
@@ +34,5 @@
> +    contentPolicyType: Ci.nsIContentPolicy.TYPE_IMAGE
> +  });
> +  let tmpLoadInfo = tmpChannel.loadInfo;
> +
> +  var decision = content_policy.shouldLoad(content_uri, tmpChannel.loadInfo);

s/tmpChannel.loadInfo/tmpLoadInfo

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1446587-shouldLoad.patch (v1b)

Thanks Christoph, we'll see whether it works when the time comes. It's always better to prepare than to do it under bustage-fix pressure.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1446587-shouldLoad.patch (v2)

Looks like the final version of ShouldLoad() has brought back the mime type, so the patch needed tweaking. I also fixed various omissions :-(

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/01811a944fad
Port Bug 1439713: Change nsIContentPolicy::ShouldLoad() to take an nsILoadInfo. rs=bustage-fix,f=ckerschb

Comment 10

[Jorg K (CEST = GMT+2)]

So far one test failure:
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_nsIMsgContentPolicy.js

16:51:48     INFO -  NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService.newChannelFromURI2]
16:51:48     INFO -  NetUtil_newChannel@resource://gre/modules/NetUtil.jsm:361:16

Looks like the NetUtil call isn't happy:
https://hg.mozilla.org/comm-central/rev/01811a944fad#l3.33

Christoph, could you please take a look.

Comment 11

[Jorg K (CEST = GMT+2)]

Looks like this

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1522366149/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_open

INFO -  SUMMARY-PASS | test-content-tab.js::setupModule
INFO -  SUMMARY-UNEXPECTED-FAIL | test-content-tab.js | test-content-tab.js::test_content_tab_open
INFO -    EXCEPTION: The tab [[object Object]] should have a favicon with URL http://localhost:43336/whatsnew.png but instead has http://localhost:43336/favicon.ico
INFO -      at: test-folder-display-helpers.js line 107
INFO -         do_throw test-folder-display-helpers.js:107 13
INFO -         mark_failure logHelper.js:649 3
INFO -         assert_content_tab_has_favicon test-content-tab-helpers.js:287 5
INFO -         test_content_tab_open test-content-tab.js:41 3

is also related. Sorry, the log for the failure from comment #10 said:
INFO -  NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService.newChannelFromURI2]
INFO -  NetUtil_newChannel@resource://gre/modules/NetUtil.jsm:361:16
INFO -  run_test@/Users/cltbld/tasks/task_1522366559/build/tests/xpcshell/tests/comm/mailnews/base/test/unit/test_nsIMsgContentPolicy.js:30:20

Comment 12

[Boris Zbarsky [:bzbarsky]]

> loadingNode: req_uri

That makes no sense.  That's supposed to be a node, but you're passing a URI.  You probably need a document whose principal has req_uri as a URI here.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks, Boris. The old code had that 'req_uri':
https://hg.mozilla.org/comm-central/rev/01811a944fad#l3.29

This one should be right though:
https://hg.mozilla.org/comm-central/rev/01811a944fad#l1.47

Comment 14

[Jorg K (CEST = GMT+2)]

Just looking on the error console I see that this also complains:
https://hg.mozilla.org/comm-central/rev/01811a944fad#l1.50

NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIIOService.newChannelFromURI2] NetUtil.jsm:361

So "arg 1" is the second argument, do they count from 0? So also here the
  loadingNode: targetDoc.documentURIObject,
is wrong. That's what Christoph suggested, see comment #3.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> This one should be right though:

It's not right, because it's passing a URI, not a node.

> That's what Christoph suggested, see comment #3.

Sure.  Christoph's suggestion is wrong.  ;)

Comment 16

[Boris Zbarsky [:bzbarsky]]

In particular, "loadingNode: targetDoc" would work when you have a targetDoc...

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> You probably need a document whose principal has req_uri as a URI here.
And how would I get that?

  var req_uri = makeURI("custom-scheme://custom_url/1.emal");
  Assert.ok(req_uri);

  var content_uri = makeURI("custom-scheme://custom_content_url/1.jsp");
  Assert.ok(content_uri);

  let tmpChannel = NetUtil.newChannel({
    uri: content_uri,
    loadingNode: req_uri,
    securityFlags: Ci.nsILoadInfo.SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK,
    contentPolicyType: Ci.nsIContentPolicy.TYPE_IMAGE
  });

There is no document nor node nor nothing, it's a URI.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> And how would I get that?

I don't know.  It depends on what this test is trying to test.

> There is no document nor node nor nothing, it's a URI.

That's all the old content policy API needed, sure.  The new API basically needs a node, because in practice all loads that are subject to content policy (i.e. loads on the web) are associated with a node.

This goes back to what this test is trying to test.  Do you actually ever have loads whose originating URI is "custom-scheme://custom_url/1.emal"?  I would think not...  So what is the point of this test?

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> So what is the point of this test?
I have no idea. The comment says:
 * Test suite for nsIMsgContentPolicy to check we could add/remove customized protocol to
 * nsMsgContentPolicy.
It does
  msg_content_policy.addExposedProtocol("custom-scheme");
and
  msg_content_policy.removeExposedProtocol("custom-scheme");
Do these not work any more? Perhaps not for unknown protocols.

I tried |loadUsingSystemPrincipal: true,| and get: NS_ERROR_UNKNOWN_PROTOCOL which is not a surprise.
I tried changing the scheme to "cid" and got NS_ERROR_ILLEGAL_VALUE, this time from nsIContentPolicy.shouldLoad.
I tried changing the scheme to "mailbox" and got (NS_ERROR_FILE_UNRECOGNIZED_PATH, this time from nsIIOService.newChannelFromURI2.

I tried changing the URLs to
mailbox://server/folder?number=1 and
ailbox://server/folder/?number=1?part=1.1&filename=evelscript.js
and then I get errors from nsMailboxProtocol.cpp, "couldn't get message header", so looks like it actually tries to access the URL.

Looks like I can't win here.

We have a realistic Mozmill test test-general-content-policy.js and that still works.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1446587-follow-up.patch

Let's disable the test for now.

Comment 21

[Jorg K (CEST = GMT+2)]

BTW, the other failure in
  mozmake SOLO_TEST=content-tabs/test-content-tab.js mozmill-one
is fixed by this patch.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3c995cbad907
Follow-up: Pass node instead of URI to NetUtil.newChannel() and disable test_nsIMsgContentPolicy.js. rs=bustage-fix

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Do these not work any more?

That should still work fine as far as it goes.

> Looks like I can't win here.

Well, to keep testing the thing this test was testing you need a node with a principal whose URI is that fake thing that's used as the source URI (the custom-scheme thing).  You can probably come up with a setup where you create a principal from the URI, then create a sandbox from the principal, then use XHR to blob URL (all this happening in the sandbox) to get a responseXML document, and then use that document as your loading node.

Comment 24

[Jorg K (CEST = GMT+2)]

Hmm, this has just become a tiny little bit more difficult ;-(

I don't think the test is buying us an awful lot, at least not in comparison to other issues we have. I filed bug 1450281 as a follow-up.

Boris, thanks for your comments.