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.

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

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.

(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);

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

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

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.

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

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

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.

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

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

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

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.

> 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. ;)

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

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

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

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

Let's disable the test for now.

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

Pushed by mozilla@jorgk.com: 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

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

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.