Last Comment Bug 769594 - Make nsContentUtils and nsObjectLoadingContent use TestPermissionFromPrincipal instead of TestPermission
: Make nsContentUtils and nsObjectLoadingContent use TestPermissionFromPrincipa...
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mounir Lamouri (:mounir)
Depends on: 769583 774819
Blocks: app-data-jars
  Show dependency treegraph
Reported: 2012-06-29 03:23 PDT by Mounir Lamouri (:mounir)
Modified: 2012-09-01 07:57 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (1.58 KB, patch)
2012-06-29 03:23 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch (2.95 KB, patch)
2012-06-29 03:28 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Review
Patch v1.1 (3.08 KB, patch)
2012-07-17 15:42 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v2 (2.83 KB, patch)
2012-07-17 18:12 PDT, Jonas Sicking (:sicking)
mounir: review+
Details | Diff | Review
Patch v3 (4.70 KB, patch)
2012-07-18 22:41 PDT, Jonas Sicking (:sicking)
jaws: review+
jaas: review+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2012-06-29 03:23:35 PDT
Created attachment 637824 [details] [diff] [review]
Comment 1 Mounir Lamouri (:mounir) 2012-06-29 03:26:05 PDT
Comment on attachment 637824 [details] [diff] [review]

I will use that bug for various content/ and dom/ changes.
Comment 2 Mounir Lamouri (:mounir) 2012-06-29 03:27:32 PDT
Actually, just too...
Comment 3 Mounir Lamouri (:mounir) 2012-06-29 03:28:47 PDT
Created attachment 637827 [details] [diff] [review]
Comment 4 Mounir Lamouri (:mounir) 2012-07-17 15:42:56 PDT
Created attachment 643168 [details] [diff] [review]
Patch v1.1
Comment 5 Jonas Sicking (:sicking) 2012-07-17 18:12:07 PDT
Created attachment 643217 [details] [diff] [review]
Patch v2
Comment 6 Mounir Lamouri (:mounir) 2012-07-18 11:35:07 PDT
Comment on attachment 643217 [details] [diff] [review]
Patch v2

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

That means we *never* gives "plugins" permissions to system principal? It might fix the failing test but that seems quite wrong.
Comment 7 Mounir Lamouri (:mounir) 2012-07-18 11:39:26 PDT
I pushed the nsContentUtils changes given that _they_ pass try obviously:
Comment 8 Mounir Lamouri (:mounir) 2012-07-18 14:08:39 PDT
Comment on attachment 643217 [details] [diff] [review]
Patch v2

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

It seems that the plugin guys actually want that behavior, so r=me.
But be careful, only land the changes in nsObjectLoadingContent.cpp ;)
Comment 9 Jonas Sicking (:sicking) 2012-07-18 14:09:19 PDT
I talked with keeler and he gave an ok over irc to go with the current patch since it maintains current behavior. I'll file a followup to figure out a policy for plugins in system-principal documents.
Comment 10 Jonas Sicking (:sicking) 2012-07-18 22:38:02 PDT
Comment on attachment 643217 [details] [diff] [review]
Patch v2

This got backed out again
Comment 11 Jonas Sicking (:sicking) 2012-07-18 22:41:08 PDT
Created attachment 643727 [details] [diff] [review]
Patch v3

This seems to pass tests.

Keeler: I tried using the http URL for all tests, but some test started throwing due to a missing function, so it seems like some of the tests depend on being loaded as chrome. So I only changed the 12* tests.

Does this look ok?
Comment 12 David Keeler [:keeler] (use needinfo?) 2012-07-18 23:37:47 PDT
Comment on attachment 643727 [details] [diff] [review]
Patch v3

The changes to the test look fine to me. The thing is, I'm not actually a reviewer (as far as I know - I'm assuming there's some sort of process or confirmation?), so you'll probably want to ask someone like Jared Wein if you need a formal review. So, I'm just going to clear the review flag.
Comment 13 Jonas Sicking (:sicking) 2012-07-18 23:52:19 PDT
Comment on attachment 643727 [details] [diff] [review]
Patch v3

Jared: Could you have a look at the test changes here.
Comment 14 Ed Morley [:emorley] 2012-07-19 07:34:18 PDT
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-07-19 10:05:25 PDT
Comment on attachment 643727 [details] [diff] [review]
Patch v3

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

Josh, can you review the nsObjectLoadingContent.cpp changes here?

r=me for the test changes, but Josh needs to sign off on this too.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +499,5 @@
>      nsCOMPtr<nsIPermissionManager> permissionManager = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    bool allowPerm = false;
> +    if (!nsContentUtils::IsSystemPrincipal(topDoc->NodePrincipal())) {

allowPerm will be false for SystemPrincipals, which I think is wrong here.

Can you change this to:
bool allowPerm = nsContentUtils::IsSystemPrincipal(topDoc->NodePrincipal());
if (!allowPerm) {
Comment 16 Josh Aas 2012-07-19 11:16:45 PDT
Comment on attachment 643727 [details] [diff] [review]
Patch v3

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

I'm fine with this, but what :jaws said.
Comment 17 Jonas Sicking (:sicking) 2012-07-19 13:11:53 PDT
I talked this over with keeler yesterday.

The patch currently maintains existing behavior. Right now the code enforces click-to-play for chrome URIs unless someone has somehow managed to get chrome URIs into nsIPermissionManager, which generally shouldn't happen.

In fact, we even have tests which depends on nsIPermissionManager not returning ALLOW for these chrome URIs.

The way that the principal-aware functions on nsIPermissionManager is that they always return ALLOW for the system principal. However that caused click-to-play tests to fail. I'm happy to change these tests, but I need help if so.

I can't find the test in question any more, the tryserver pushes are too old. But I can re-push to get results again.

However, is it really worth making plugins not use click-to-play in chrome documents given that it seems like there's agreement that plugins shouldn't work in chrome documents at all?
Comment 18 Jonas Sicking (:sicking) 2012-07-19 13:13:22 PDT
Comment on attachment 643727 [details] [diff] [review]
Patch v3

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

re-asking for review. See comment above.
Comment 19 Josh Aas 2012-07-19 13:52:06 PDT
Comment on attachment 643727 [details] [diff] [review]
Patch v3

I'm fine with not dragging you into changing the existing behavior, but we should get to that sooner rather than later.
Comment 20 Jared Wein [:jaws] (please needinfo? me) 2012-07-20 07:35:18 PDT
Jonas, can you add a comment to nsObjectLoadingContent.cpp to state why SystemPrincipals default to allow=false?
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-07-21 14:43:04 PDT

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