296311 - Cannot use drawWindow method on canvas on local page with enablePrivilege code

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Martijn Wargers (dead)]

I tried to use the new drawWindow method on the canvas.
See testcase.
That testcase uses enablePrivilege, so you have to test it locally on the
computer to test if it works. Currently it does not, but I think it should work
at least with one form of the enablePrivilege code.

This is what Boris said about it on IRC:
	bz	{#developers} mw22: the relevant code in
nsCanvasRenderingContext2D::DrawWindow is:
	bz	1767 PRBool isChrome = PR_FALSE;
	bz	1768 ssm->SubjectPrincipalIsSystem(&isChrome);
	bz	1769 if (!isChrome)
	bz	1770 return NS_ERROR_FAILURE;
	bz	{#developers} mw22: well. we need the security check in some form.
	bz	{#developers} mw22: probably IsCapabilityEnabled("UniversalBrowserRead") is
what we want to be checking
	bz	{#developers} mw22: I think UniversalBrowserRead is a reasonable thing to
check there; of you have it, you can get the data anyway

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

To be sure, I attach the url testcase here. 
It might be that the testcase needs adjusting (which enablePrivilege to use),
depending on how this bug gets fixed.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: drawwindow-perms.patch

Add check for UniversalBrowserRead or UniversalXPConnect privs; also fixes a
call to cairo_set_source_surface that's making DrawWindow unusable on the trunk
(bug 296697).

Comment 3

[Stuart Parmenter]

Comment on attachment 185441 [details] [diff] [review]
drawwindow-perms.patch

Change the 3 if's to use || and r=pavlov

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 185441 [details] [diff] [review]
drawwindow-perms.patch

So why do we have !isTrusted in the RenderOffscreen call, if isTrusted can only
be true if we get there?

Also, I'd rather see an explicit

if (!isTrusted)
    return NS_ERROR_DOM_SECURITY_ERR;

Relying on the else-chaining seems fragile; it'd be easy to add another chunk
in there and not realize that it was a security-grant even if you don't set
isTrusted.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 185441 [details] [diff] [review]
drawwindow-perms.patch

a=shaver with changes from IRC.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

in with fixes, thanks

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The reason for the isTrusted thing is so we can add untrusted rendering later
without changing the API.

Comment 8

[Martijn Wargers (dead)]

It works, it works!!! Thank you! :-)