605140 - Composition content policy broken due to compartments landing

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: Proposed fix

When the js compartments landed, the compose content policy was broken. This was hidden initially because of bug 604493, however, once that gets landed we'll be able to see it.

The compose part of Mailnews' content policy basically denies loading any remote content within emails being replied or forwarded where the user has not allowed loading of remote content on the original email.

This used to work by:

- Each compose window would store a reference to its nsIDOMWindowInternal in nsIMsgComposeService.
- The content policy would then get the nsIDOMWindowInternal from the root docShell
- The content poilcy would then pull the nsIDOMWindowInternal out from the nsIMsgComposeService.

This worked fine before as "window" in javascript was storing a reference to the outer window. However, now it seems to store a reference to the inner window (see bug 604493 comment 18).

Proposed Solution:

- Change the code to store docShells rather than windows.
- The docShells would be the actual <editor> element.

The advantage of using docShells of the actual editor element is that we can then register any editor element in any window as a "compose" element. This gives us flexibility later on for compose in a tab or other compose experiments.

Combined with the patch on bug 604493 this makes our content policy tests pass again. These our fairly extension wrt remote content in the compose window.


Requesting feedback from bz on the general idea of using docShells versus windows for the content policy.

Comment 1

[Mark Banner (:standard8)]

Note: I failed to update a few test files and one Windows file to account for the change in interface, but the patch is still mainly valid - I'll attach a new one tomorrow.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 483978 [details] [diff] [review]
Proposed fix

Using docshells seems fine, but I still wonder whether we should keep making the window that appears on the C++ side when |window| is passed in to XPCOM methods the outer window...  It seems like there's a lot more breakage like this lurking.  :(

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix

Full fix including the files I'd missed updating previously. Needs Neil's patch from bug 604493 to work fully.

Comment 4

[[e-mail]]

Comment on attachment 484286 [details] [diff] [review]
The fix

>+  nsIMsgCompose* msgCompose = nsnull;
>+  rv = composeService->GetMsgComposeForDocShell(docShell, &msgCompose);
>+  // GetMsgComposeForDocShell returns NS_ERROR_FAILURE for not found. So don't
>+  // warn about it.
>+  if (NS_FAILED(rv))
>+    return nsnull;
>+
>+  return msgCompose;
[Belt-and-braces approach here; because GetMsgComposeForFocShell won't write to msgCompose on failure, you can simplify this either by unconditionally returning it or by just not initialising it.]

Comment 5

[David :Bienvenu]

Comment on attachment 484286 [details] [diff] [review]
The fix

nit:

+  nsCOMPtr<nsIDocShellTreeItem> docshellTreeItem(do_QueryInterface(shell, &rv));

everywhere else, we capitalize it thus: docShell

xpcshell and mozmill tests all pass for me, with the exception of test-attachment-reminder, which doesn't seem to work at all. I'll try a manual test as well.

Comment 6

[David :Bienvenu]

Comment on attachment 484286 [details] [diff] [review]
The fix

I suppose the attachment and signature stuff belongs in its own bug.

Comment 7

[Mark Banner (:standard8)]

Pushed with fixes for the comments:

http://hg.mozilla.org/comm-central/rev/0c978b955130

Leaving open as the tests won't run fully until bug 604493 lands, but I wanted to land this in advance so we could clean the tree up quicker when we do get approval for that one.

Comment 8

[Mark Banner (:standard8)]

All is ok now that the core patch has landed. Marking fixed, and setting in-testsuite+ as this bug is covered by existing tests.