Last Comment Bug 781126 - AllowPlugins = false on docshell is no longer working ? | Thunderbird Permanent TEST-UNEXPECTED-FAIL | test-plugins-policy.js | Plugin has not been blocked in message as expected
: AllowPlugins = false on docshell is no longer working ? | Thunderbird Permane...
Status: RESOLVED FIXED
: intermittent-failure
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- blocker (vote)
: mozilla17
Assigned To: John Schoenick [:johns]
:
Mentors:
Depends on:
Blocks: 309524 framesandbox 745030
  Show dependency treegraph
 
Reported: 2012-08-08 03:59 PDT by Mark Banner (:standard8)
Modified: 2012-11-25 19:31 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Ensure plugins always pass through shouldLoad, add additional check for shouldProcess (8.06 KB, patch)
2012-08-10 16:59 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Ensure plugins always pass through shouldLoad, add additional check for shouldProcess (8.08 KB, patch)
2012-08-10 17:03 PDT, John Schoenick [:johns]
jaas: review+
Details | Diff | Splinter Review
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad (9.20 KB, patch)
2012-08-16 18:57 PDT, John Schoenick [:johns]
jst: review+
bzbarsky: feedback+
john: checkin+
Details | Diff | Splinter Review
Part 2 - nsObjectLoadingContent should call shouldLoad and shouldProcess sanely (13.90 KB, patch)
2012-08-16 18:57 PDT, John Schoenick [:johns]
jst: review+
john: checkin+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2012-08-08 03:59:36 PDT
Since bug 745030 landed, we've been getting permanent test failures on Thunderbird:

TEST-START | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test_3paneWindowDenied
Test Failure: Plugin has not been blocked in message as expected
NPP_Destroy
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied

https://tbpl.mozilla.org/php/getParsedLog.php?id=14218472&tree=Thunderbird-Trunk#error0

The regression point is the first changeset of that bug, namely:

http://hg.mozilla.org/mozilla-central/rev/40a8737b62a1
Comment 1 Mark Banner (:standard8) 2012-08-08 04:08:54 PDT
Here's the test for Thunderbird:

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-policy/test-plugins-policy.js

What is happening in the test, and how Thunderbird handles it is this:

- Thunderbird loads an email with a plugin in it.
- nsMsgContentPolicy::ShouldLoad [1] gets called multiple times, but one of those is with a content type of nsIContentPolicy::TYPE_DOCUMENT
- At that point, Thunderbird sets allowPlugins to false on the docshell [2]
- The load of the message then proceeds
- The test calls .setColor() on the plugin element to test if the plugin was loaded or not (similar to some of the core tests)

Expected results:

- Message loaded, but plugin load denied, test pass

Actual results:

- Message loaded, plugin is also loaded and accessible, test fails

In doing debug, I can see that the docshell gets allowPlugins set to false, and a little later the plugin gets loaded, hence why I'm concluding that the docshell blocking appears to not be working any more.

[1] http://hg.mozilla.org/comm-central/annotate/8c305d4656a7/mailnews/base/src/nsMsgContentPolicy.cpp#l161
[2] http://hg.mozilla.org/comm-central/annotate/8c305d4656a7/mailnews/base/src/nsMsgContentPolicy.cpp#l687
Comment 2 Treeherder Robot 2012-08-08 04:23:45 PDT
Standard8
https://tbpl.mozilla.org/php/getParsedLog.php?id=14218938&tree=Thunderbird-Trunk
TB Rev4 MacOSX Lion 10.7 comm-central debug test mozmill on 2012-08-08 02:16:17
slave: talos-r4-lion-027

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 3 Treeherder Robot 2012-08-08 04:46:23 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14219756&tree=Thunderbird-Try
TB Rev3 WINNT 5.1 try-comm-central opt test mozmill on 2012-08-08 03:20:07
slave: talos-r3-xp-020

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 4 Treeherder Robot 2012-08-08 04:47:35 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14221402&tree=Thunderbird-Try
TB Rev3 WINNT 6.1 try-comm-central opt test mozmill on 2012-08-08 04:16:33
slave: talos-r3-w7-073

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 5 Benjamin Smedberg [:bsmedberg] 2012-08-08 05:53:23 PDT
I think this should block 17, I believe that this attribute is sometimes used by extensions in Firefox.
Comment 6 Boris Zbarsky [:bz] 2012-08-08 09:45:34 PDT
Is the problem is that the NS_CheckProcessPolicy call went away?  If so, presumably this regressed bug 393503 as well.  It's unfortunate that I never figured out how to write a test for that one.  :(
Comment 7 Mark Banner (:standard8) 2012-08-08 10:34:21 PDT
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #6)
> Is the problem is that the NS_CheckProcessPolicy call went away?  If so,
> presumably this regressed bug 393503 as well.  It's unfortunate that I never
> figured out how to write a test for that one.  :(

Do you mean NS_CheckContentProcessPolicy ?
Comment 8 Boris Zbarsky [:bz] 2012-08-08 11:10:57 PDT
Er, yes.
Comment 9 Treeherder Robot 2012-08-10 06:44:02 PDT
Standard8
https://tbpl.mozilla.org/php/getParsedLog.php?id=14279292&tree=Thunderbird-Trunk
TB Rev3 Fedora 12 comm-central opt test mozmill on 2012-08-09 19:28:19
slave: talos-r3-fed-044

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/message-window/test-commands.js | test-commands.js::test_copy_eml_message
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 10 Ian Melven :imelven 2012-08-10 13:45:58 PDT
This bug breaks blocking plugins in the patches for iframe sandbox in bug 341604.
Comment 11 Ian Melven :imelven 2012-08-10 16:06:06 PDT
I tried to take a crack at this - I added the NS_CheckContentProcessPolicy call back in somewhere around where it used to be, but plugins still aren't blocked and this check actually passes in my test case.

I'm debugging some more to try to understand what's going on :)
Comment 12 Ian Melven :imelven 2012-08-10 16:11:46 PDT
Looks like checkURILoad isn't being called by CheckObjectURIs for the plugins that should be blocked, because they have a null mURI, without this, the docshell/document never get asked if plugins are enabled.
Comment 13 Ian Melven :imelven 2012-08-10 16:24:33 PDT
It looks like mURI is set for a document with a content type that should be handled by a plugin but not for a plugin loaded by an embed or object..
Comment 14 Ian Melven :imelven 2012-08-10 16:33:20 PDT
Oops, didn't mean to remove that this blocks 745030
Comment 15 Ian Melven :imelven 2012-08-10 16:33:59 PDT
Oh bugzilla UI.
Comment 16 John Schoenick [:johns] 2012-08-10 16:59:41 PDT
Created attachment 651053 [details] [diff] [review]
Ensure plugins always pass through shouldLoad, add additional check for shouldProcess
Comment 17 John Schoenick [:johns] 2012-08-10 17:03:48 PDT
Created attachment 651055 [details] [diff] [review]
Ensure plugins always pass through shouldLoad, add additional check for shouldProcess
Comment 18 John Schoenick [:johns] 2012-08-10 17:05:50 PDT
Comment on attachment 651055 [details] [diff] [review]
Ensure plugins always pass through shouldLoad, add additional check for shouldProcess

So we never pass through shouldLoad() or shouldProcess() with no URI, which is obviously wrong

This checks shouldLoad on the document's base URI when the plugin has no URI of its own, so we can do type based blocking, and adds a shouldProcess call once the final type is known as well. However, due to bug 380556, the latter isn't so productive.
Comment 19 Boris Zbarsky [:bz] 2012-08-12 09:36:37 PDT
Calling shouldLoad twice on the same load (as described in bug 309524) isn't right.  We didn't use to do that, iirc; why do we need to do it now?
Comment 20 John Schoenick [:johns] 2012-08-12 12:32:55 PDT
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #19)
> Calling shouldLoad twice on the same load (as described in bug 309524) isn't
> right.  We didn't use to do that, iirc; why do we need to do it now?

A) We don't know the final type when we first call should load, and we have blockers that, say, always reject TYPE_OBJECT loads - but the tag may well be loading an image
B) shouldProcess is not used properly (see bug 380556 / http://dxr.mozilla.org/mozilla-central/embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp.html#l97 )

If we fixed items blocking on shouldLoad that should be blocking on shouldProcess, and actually used shouldProcess, we could remove the double shouldLoad calls
Comment 21 Boris Zbarsky [:bz] 2012-08-12 13:40:07 PDT
> We don't know the final type when we first call should load, and we have blockers that,
> say, always reject TYPE_OBJECT loads - but the tag may well be loading an image

The blockers that I know of that always reject TYPE_OBJECT do it because the load can do dangerous things before you even find out whether it's really an image or not.  Which is why we have to always do that first check before we ever call AsyncOpen.

> shouldProcess is not used properly 

Its not used _consistently_ (which is what the bug you cite is about).  As in, we don't call it all the places where we should.  But in this place it _is_ called, and consumers know to look for it here.

And doing shouldLoad twice would actually break those consumers last I checked.

Again, how would just doing the shouldLoad before we asyncOpen and doing shouldProcess before handling the data differ from what we used to do before bug 745030?
Comment 22 Boris Zbarsky [:bz] 2012-08-12 13:40:47 PDT
Or is the problem you're running into that nsWebBrowserContentPolicy specifically ignores ShouldProcess?  If so, how did this use to work?
Comment 23 John Schoenick [:johns] 2012-08-12 14:20:45 PDT
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #21)
> The blockers that I know of that always reject TYPE_OBJECT do it because the
> load can do dangerous things before you even find out whether it's really an
> image or not.  Which is why we have to always do that first check before we
> ever call AsyncOpen.

What dangerous things, in this case? We don't begin doing anything with the data until we've re-checked shouldLoad with the proper type. That method also means that trying to block images or subdocuments will fail unless you also block type object.

> Again, how would just doing the shouldLoad before we asyncOpen and doing
> shouldProcess before handling the data differ from what we used to do before
> bug 745030?

> Or is the problem you're running into that nsWebBrowserContentPolicy
> specifically ignores ShouldProcess?  If so, how did this use to work?

Previously we would only ever call shouldProcess for objects that loaded via channels, which many plugins do not - you can load flash without a URI, java never has a URI, etc.. The actual blocking, then, depended on calling shouldLoad() with TYPE_OBJECT - regardless of what type we'd end up loading. Some consumers even tried to work around this, but incorrectly [1]

With the way it works after this patch, we would call shouldLoad() for type object, but if rejected, see if it passes with image or subdocument instead. If the channel then changes our type to something else, we re-call shouldLoad with the proper type. This second shouldLoad call is superfluous, but needed because of the aforementioned nsWebBrowserContentPolicy bug. We finally call shouldProcess once with the proper type (which, for the consumers I checked in the browser, accomplishes nothing, but is proper)

I agree that this is ugly, but the result is that if you have a content policy that responds correctly to shouldLoad or shouldProcess, it will work as expected without tricks like [1], and current arguably-wrong content policy in the browser will continue to work right.

> And doing shouldLoad twice would actually break those consumers last I checked.

shouldLoad seems to me to be a "here's a URI and type, should I load it?" question posed to the API - it is not a guarantee we will immediately load that uri or that we wont ask to load that uri as a different type later -- what consumers would this cause issues with, specifically?


[1] http://dxr.mozilla.org/mozilla-central/extensions/permissions/nsContentBlocker.cpp.html#l176
( this should actually be removed after this patch lands )
Comment 24 Boris Zbarsky [:bz] 2012-08-12 16:18:49 PDT
> What dangerous things, in this case?

Well, for example <object data="javascript:whatever"> will run the script in the context of the page before it has any idea what the resulting data looks like.

> That method also means that trying to block images or subdocuments will fail unless you
> also block type object.

Right, unless we correctly call ShouldProcess.  I'm not saying this stuff is bug-free, exactly.  ;)

> The actual blocking, then, depended on calling shouldLoad() with TYPE_OBJECT

OK.  And we're still doing that, right?

> we would call shouldLoad() for type object, but if rejected, see if it passes with
> image or subdocument instead.

I'm not sure this is a great idea: it assumes shouldLoad calls are idempotent, and I'm not sure that assumption is correct with things like adblock and whatnot.

> This second shouldLoad call is superfluous, but needed because of the aforementioned
> nsWebBrowserContentPolicy bug.

Ah, so this is only needed because now we do shouldLoad calls with image/subdocument types too before starting the load?  Was that added in bug 745030, or is it being added in this patch?

> what consumers would this cause issues with, specifically?

Various extensions block loads but provide UI to start them later (e.g. flashblock).  So they end up storing various metadata per shouldLoad call and making multiple such calls can confuse them.  I know some extensions ended up having to add code to deal with the extra shouldLoad calls from image preloading...

What I would prefer here is that we move to a model where we call shouldLoad once before we start loading and then shouldProcess once before processing and adjust any in-tree content policy impls that can't deal with that.  And that means calling shouldProcess for all types, not just plug-ins, and for plug-ins in all cases.

If this is too high-risk or has other difficulties, then my second preference would be restoring the call sequence we used to have (which we we know "works" in the sense of not introducing new issues) while we sort those difficulties out.
Comment 25 Treeherder Robot 2012-08-13 10:28:54 PDT
Standard8
https://tbpl.mozilla.org/php/getParsedLog.php?id=14341171&tree=Thunderbird-Try
TB Rev3 Fedora 12 try-comm-central opt test mozmill on 2012-08-13 07:45:54
slave: talos-r3-fed-006

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 26 Treeherder Robot 2012-08-14 02:22:54 PDT
Standard8
https://tbpl.mozilla.org/php/getParsedLog.php?id=14363885&tree=Thunderbird-Trunk
TB Rev3 Fedora 12 comm-central opt test mozmill on 2012-08-14 01:06:48
slave: talos-r3-fed-034

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 27 Treeherder Robot 2012-08-14 05:38:40 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14367884&tree=Thunderbird-Trunk
TB Rev3 Fedora 12 comm-central opt test mozmill on 2012-08-14 04:34:28
slave: talos-r3-fed-049

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/attachment/test-attachment.js | test-attachment.js::test_attachment_right_click_single
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 28 Treeherder Robot 2012-08-14 05:40:49 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14367850&tree=Thunderbird-Trunk
TB Rev3 Fedora 12x64 comm-central opt test mozmill on 2012-08-14 04:38:05
slave: talos-r3-fed64-025

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/message-window/test-commands.js | test-commands.js::test_copy_eml_message
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 29 Treeherder Robot 2012-08-14 05:40:55 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14367967&tree=Thunderbird-Trunk
TB Rev4 MacOSX Snow Leopard 10.6 comm-central opt test mozmill on 2012-08-14 04:46:15
slave: talos-r4-snow-056

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 30 Treeherder Robot 2012-08-14 05:41:18 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14368380&tree=Thunderbird-Trunk
TB Rev4 MacOSX Snow Leopard 10.6 comm-central debug test mozmill on 2012-08-14 04:42:21
slave: talos-r4-snow-051

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 31 Treeherder Robot 2012-08-14 05:41:24 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14368739&tree=Thunderbird-Trunk
TB Rev3 Fedora 12x64 comm-central debug test mozmill on 2012-08-14 04:47:32
slave: talos-r3-fed64-062

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 32 Treeherder Robot 2012-08-14 05:41:34 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14367974&tree=Thunderbird-Trunk
TB Rev4 MacOSX Lion 10.7 comm-central opt test mozmill on 2012-08-14 04:46:15
slave: talos-r4-lion-074

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 33 Treeherder Robot 2012-08-14 05:41:38 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14368464&tree=Thunderbird-Trunk
TB Rev4 MacOSX Lion 10.7 comm-central debug test mozmill on 2012-08-14 04:42:17
slave: talos-r4-lion-051

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 34 Treeherder Robot 2012-08-14 06:39:01 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14369903&tree=Thunderbird-Trunk
TB Rev3 WINNT 6.1 comm-central opt test mozmill on 2012-08-14 06:03:31
slave: talos-r3-w7-063

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 35 Treeherder Robot 2012-08-14 06:39:05 PDT
Aryeh Gregor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14369970&tree=Thunderbird-Trunk
TB Rev3 WINNT 5.1 comm-central opt test mozmill on 2012-08-14 06:11:47
slave: talos-r3-xp-037

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 36 Treeherder Robot 2012-08-14 06:39:58 PDT
Standard8
https://tbpl.mozilla.org/php/getParsedLog.php?id=14369903&tree=Thunderbird-Trunk
TB Rev3 WINNT 6.1 comm-central opt test mozmill on 2012-08-14 06:03:31
slave: talos-r3-w7-063

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\mozmill\content-policy\test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 37 John Schoenick [:johns] 2012-08-16 18:57:40 PDT
Created attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad
Comment 38 John Schoenick [:johns] 2012-08-16 18:57:51 PDT
Created attachment 652647 [details] [diff] [review]
Part 2 - nsObjectLoadingContent should call shouldLoad and shouldProcess sanely
Comment 39 John Schoenick [:johns] 2012-08-16 19:02:04 PDT
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

Well lets just fix content policy then

This changes nsWebBrowserContentPolicy to enforce docshell.allowPlugins in shouldProcess, rather than shoudLoad, since the final object type is unknown at that point.

It further changes nsContentBlocker to do the same, removing old ugly hacks that introspected nsObjectLoadingContent to try to "guess" at a type (which was wrong, in many ways)

BONUS: I may have fixed a case wherein click-to-play was being inadvertently bypassed when explicit permissions were set -- nsContentBlocker does not take into account object tags with a null URI (which can happen with java or even weird cases of flash)
Comment 40 John Schoenick [:johns] 2012-08-16 19:04:09 PDT
Comment on attachment 652647 [details] [diff] [review]
Part 2 - nsObjectLoadingContent should call shouldLoad and shouldProcess sanely

With part 1 landed, we're free to just call shouldLoad(TYPE_OBJECT) before opening a URI, and shouldProcess(PROPER_TYPE) afterwards. We also don't need to worry about mURI being null, since content policy handles that correctly now.
Comment 41 John Schoenick [:johns] 2012-08-16 19:35:22 PDT
https://tbpl.mozilla.org/?tree=Try&rev=46b32a4de584
Comment 42 Treeherder Robot 2012-08-17 12:25:50 PDT
mkmelin
https://tbpl.mozilla.org/php/getParsedLog.php?id=14475019&tree=Thunderbird-Trunk
TB Rev3 Fedora 12 comm-central opt test mozmill on 2012-08-17 11:21:05
slave: talos-r3-fed-044

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 43 Treeherder Robot 2012-08-17 12:26:27 PDT
mkmelin
https://tbpl.mozilla.org/php/getParsedLog.php?id=14475569&tree=Thunderbird-Trunk
TB Rev3 Fedora 12x64 comm-central opt test mozmill on 2012-08-17 11:45:35
slave: talos-r3-fed64-005

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDenied
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_3paneWindowDeniedAgain
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/content-policy/test-plugins-policy.js | test-plugins-policy.js::test_checkStandaloneMessageWindowDenied
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Comment 44 Johnny Stenback (:jst, jst@mozilla.com) 2012-08-17 14:17:45 PDT
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

Looks good to me, though I spotted two minor typos below. And we should have bz look these changes over too before we ship this, but feel free to land and follow up with bz after the fact.

- In nsContentBlocker::ShouldLoad():

+  // The final type of an object tag mate mutate before it reaches

s/mate/may/

- In nsContentBlocker::ShouldProcess():

+  // For objects, we only check policy in shouldProcess, as the final type isnt

s/isnt/isn't/
Comment 47 :Ms2ger (⌚ UTC+1/+2) 2012-08-18 02:01:27 PDT
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

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

::: extensions/permissions/nsContentBlocker.cpp
@@ +212,5 @@
> +    if (!shouldLoad) {
> +      if (fromPrefs) {
> +	*aDecision = nsIContentPolicy::REJECT_TYPE;
> +      } else {
> +	*aDecision = nsIContentPolicy::REJECT_SERVER;

Tabs :(
Comment 48 Robert Kaiser 2012-08-19 08:33:41 PDT
Also, it seems this has regressed bug 781265. :(
Comment 49 John Schoenick [:johns] 2012-08-20 14:00:19 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #48)
> Also, it seems this has regressed bug 781265. :(

The newer regression ranges are now pointing towards bug 775965 for that
Comment 50 John Schoenick [:johns] 2012-08-22 14:17:11 PDT
(In reply to :Ms2ger from comment #47)
> Tabs :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/004b8c1500c2
Comment 51 Ed Morley [:emorley] 2012-08-23 03:46:52 PDT
https://hg.mozilla.org/mozilla-central/rev/004b8c1500c2
Comment 52 John Schoenick [:johns] 2012-08-23 16:50:42 PDT
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

bz - this landed while you were away to unblock iframe sandboxes, but jst wanted me to ping you and make sure you're okay with this approach
Comment 53 Boris Zbarsky [:bz] 2012-08-24 14:51:37 PDT
Comment on attachment 652646 [details] [diff] [review]
Part 1 - Browser content policy should check TYPE_OBJECT at shouldProcess rather than shouldLoad

I think this is OK, as long as nsContentBlocker is not supposed to prevent the network access...

Some nits: "mate mutate" should be "may mutate", "isnt" needs an apostrophe, and the indent on the second line of the TestPermission call in nsContentBlocker::ShouldProcess is somewhat off.

You should probably explicitly reject if aContentLocation is null instead of relying on the undocumented behavior of TestPermission doing so.
Comment 54 Boris Zbarsky [:bz] 2012-08-31 23:24:40 PDT
John, did you see comment 53?

Note You need to log in before you can comment on or make changes to this bug.