505847 - Electrolysis + tilebrowser (async .drawWindow)

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

Stuart has a test app for tilebrowser which I've committed to toolkit/content/tests/fennec-tile-testapp in the Electrolysis repo. We need this to work with a separate process driving the content process.

I think this is going to involve asynchronously shipping pieces of graphics data around. Right now we don't have shared-memory support so I think we should ship it across the pipe for the initial work.

Comment 1

[Benjamin Smedberg]

The important parts of this testapp are being put back upstream into Fennec in the next week or so: we should consider what changes we need to make in the testapp and how we want to get them back upstream into fennec and mozilla-central: do we want a new .drawWindowAsync API?

Comment 2

[Joe Drew (not getting mail)]

Attachment: add asyncDrawXULElement to canvas 2D api

This patch adds an asyncDrawXULElement method to the 2D canvas API that dispatches a request to render a window cross-process. This instantiates a new sub-protocol of the iframe protocol that sends the request across the IPC pipe. The width, height and actual image data are then returned in that protocol object's destructor, and the image is drawn to the canvas it was bound to.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't see PDocumentRenderer.ipdl in the patch, although it's referenced in a few places.  Need to |hg add| it maybe?

Comment 4

[Joe Drew (not getting mail)]

Attachment: add missing files

Forgot to hg add all the files I added - whoops.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 404877 [details] [diff] [review]
add missing files

Sorry Joe, there's too much in your patch that I understand barely or not at all.  I think bsmedberg is a better man for this job.

Comment 6

[Benjamin Smedberg]

Comment on attachment 404877 [details] [diff] [review]
add missing files

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp

>+mozilla::dom::PIFrameEmbeddingParent *

nit, style guide snuggles the * now.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>+#ifdef MOZ_IPC
>+  mozilla::dom::PIFrameEmbeddingParent *GetChildProcess();
>+#endif

ditto.

>diff --git a/content/canvas/public/Makefile.in b/content/canvas/public/Makefile.in

>+		DocumentRendererChild.h \
>+		DocumentRendererParent.h \

This should be in EXPORTS_mozilla_ipc, no?

>diff --git a/content/canvas/src/DocumentRendererChild.cpp b/content/canvas/src/DocumentRendererChild.cpp
>+static void
>+FlushLayoutForTree(nsIDOMWindow* aWindow)

I'm not qualified to review this function. I think that's probably ok, since we're going to have to review the merge before this ends up in mozilla-central. How much of this code can be split apart so that we can land the new API call in mozilla-central without regard to the IPC bits?

>+DocumentRendererChild::RenderDocument(nsIDOMWindow *window, const PRInt32& x, const PRInt32& y, const PRInt32& 

>+    nscolor bgColor;
>+    nsCOMPtr<nsICSSParser> parser = do_CreateInstance("@mozilla.org/content/css-parser;1");
>+    nsresult rv = parser->ParseColorString(PromiseFlatString(aBGColor),
>+					   nsnull, 0, &bgColor);

Boy, is there no static helper function which can parse colors without doing the XPCOM dance and creating a CSS parser object?

>+    printf("out x: %f y: %f width: %f height: %f\n", nsPresContext::AppUnitsToFloatCSSPixels(x), nsPresContext::AppUnitsToFloatCSSPixels(y), nsPresContext::AppUnitsToFloatCSSPixels(w), nsPresContext::AppUnitsToFloatCSSPixels(h));

leftover debugging code?

>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp

>+#include "mozilla/ipc/PDocumentRendererParent.h"
>+#include "mozilla/dom/PIFrameEmbeddingParent.h"
>+#include "DocumentRendererParent.h"

Need MOZ_IPC ifdefs here?

>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::AsyncDrawXULElement(nsIDOMXULElement* aElem, float aX, float aY,
>+                                       float aW, float aH, 
>+                                       const nsAString& aBGColor,
>+                                       PRUint32 flags)
>+{
>+    NS_ENSURE_ARG(aElem != nsnull);
>+
>+    nsXULElement *xulelem = static_cast<nsXULElement *>(aElem);

I don't think that's safe: right now a user could pass any arbitrary JS object in... without some sort of QI/validation this is likely to crash.

nit, need snuggly *

>+    nsFrameLoader *frameloader = static_cast<nsFrameLoader *>(iframeloader);

This is probably safe, but have you asked bz?

>+    // We can't allow web apps to call this until we fix at least the
>+    // following potential security issues:
>+    // -- rendering cross-domain IFRAMEs and then extracting the results
>+    // -- rendering the user's theme and then extracting the results
>+    // -- rendering native anonymous content (e.g., file input paths;
>+    // scrollbars should be allowed)
>+    if (!nsContentUtils::IsCallerTrustedForRead()) {
>+      // not permitted to use DrawWindow
>+      // XXX ERRMSG we need to report an error to developers here! (bug 329026)
>+        return NS_ERROR_DOM_SECURITY_ERR;
>+    }

Is there any reason not to push this up to the top of the function?

>diff --git a/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl

Need uuid rev.

>diff --git a/dom/ipc/PDocumentRenderer.ipdl b/dom/ipc/PDocumentRenderer.ipdl

>+ * The Initial Developer of the Original Code is
>+ *   Joe Drew <[e-mail]>.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.

s/Joe Drew/The Mozilla Foundation/

>diff --git a/dom/ipc/PIFrameEmbedding.ipdl b/dom/ipc/PIFrameEmbedding.ipdl

> using PRUint32;
>+using PRInt32;
>+using PRBool;

I don't think we should introduce PRBool here... can we just use `bool`?

>+using nsTArray<PRUint8>;

We have been using `nsCString` for buffers of arbitrary data (graphics/network): it serializes more efficiently because you don't have a loop for serializing each element separately but can serialize/deserialize the buffer all in one go.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>+bool
>+TabChild::RecvPDocumentRendererConstructor(
>+        mozilla::ipc::PDocumentRendererChild *__a,
>+        const PRInt32& aX,
>+        const PRInt32& aY,
>+        const PRInt32& aW,
>+        const PRInt32& aH,
>+        const nsString& bgcolor,
>+        const PRUint32& flags,
>+        const PRBool& flush)
>+    nsCOMPtr<nsIWebBrowser> browser = do_QueryInterface(mWebNav);
>+    if (!browser)
>+        return false;
>+
>+    nsCOMPtr<nsIDOMWindow> window;
>+    if (NS_FAILED(browser->GetContentDOMWindow(getter_AddRefs(window))) ||
>+        !window)
>+    {
>+        return false;
>+    }
>+
>+    PRUint32 width, height;
>+    nsTArray<PRUint8> data;
>+    nsresult rv = render->RenderDocument(window, aX, aY, aW, aH, bgcolor, flags, flush,
>+                                         width, height, data);
>+    if (NS_FAILED(rv))
>+        return false;
>+
>+    return SendPDocumentRendererDestructor(__a, width, height, data);
>+}

Returning `false` from a message will cause the content process to abort. I'm pretty sure that's not what you want here, right? I'm not exactly sure what to do about the error conditions, but we should consider either silently ignoring them or sending an explicit error message.

>diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp
>diff --git a/modules/libpr0n/src/imgRequest.cpp b/modules/libpr0n/src/imgRequest.cpp

>+#include <time.h>

extraneous change?

I really don't want to re-review this, so marking r+ and you get to deal with the comments as you will... file followups as necessary and make sure they block bug 516521

Comment 7

[Boris Zbarsky [:bzbarsky]]

> static helper function which can parse colors

That depends on how restricted your color string is.  How restricted is it in this case?

Comment 8

[Boris Zbarsky [:bzbarsky]]

And in particular do you want to allow rgba()/hsla()/rgb()/hsl()?  System colors?  Named colors?

I'd rather add noscript/notxpcom methods/properties to the frameloader interface than have the casts.  For that matter, I'd rather you used QI to nsIContent and nsXULElement::FromContent instead of casting to go from an nsIDOMXULElement (which can be any JSObject) to an nsXULElement.

Comment 9

[Joe Drew (not getting mail)]

(In reply to comment #8)
> And in particular do you want to allow rgba()/hsla()/rgb()/hsl()?  System
> colors?  Named colors?

It's very highly restricted. This is only used in frontend code, so we can limit it arbitrarily. (Right now I think it says "gray" or something like that.)

Comment 10

[Joe Drew (not getting mail)]

Attachment: interdiff of changes

This is an interdiff for the changes I made to fulfil bsmedberg's review comments.

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 406110 [details] [diff] [review]
interdiff of changes

ignore: doesn't compile and isn't complete.

Comment 12

[Joe Drew (not getting mail)]

Attachment: address review comments

This patch should address all review comments.

Boris, the reason I flagged you for review is for the changes in nsCanvasRenderingContext2D::AsyncDrawXULElement - specifically the QI and whether it's safe to do the static_cast from nsIFrameLoader to nsFrameLoader.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> It's very highly restricted.

Depending on what you want, NS_ColorNameToRGB plus NS_HexToRGB might do what you want, then.

The QI looks ok (though why not just QI to nsIFrameLoaderOwner if all you want is GetFrameLoader?).  I already commented in comment 8 about the casting, no?

Oh, and this leaks the frameloader, of course.  nsCOMPtr, if you please!

Comment 14

[Joe Drew (not getting mail)]

Attachment: address reivew comments, again

This patch adds childProcess as a noscript attribute to nsIFrameLoader, and properly uses nsIFrameLoader and nsIFrameLoaderOwner (and nsCOMPtr!!!) to get the PIFrameEmbeddingParent*.

Comment 15

[Benjamin Smedberg]

Attachment: Allow asyncDrawXULWindow to work for local browsers as well as remote

Joe, I'm using this patch on top of one of your previous ones: it allows the asyncDrawXULElement API to be used for either remote or local frames, which is what we want, I think. That way we can just change fennec to use this API unconditionally without worry which way the IPC pref is flipped.

Comment 16

[Joe Drew (not getting mail)]

Benjamin, I'll roll something along those lines into a follow-up patch. I like it a lot better than what I had planned for using asyncDrawXULElement in a non-IPC branch.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Joe, your patch bitrotted in two places --- I have an updated local version if you'd like it.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: same as Joe's patch, but builds with latest electrolysis

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK.  The new code looks like it should be ok; two questions:

1)  Why do you need to include (and export!) nsXULElement.h?
2)  Since in the interim nsIFrameLoaderOwner has grown this:

  [noscript, notxpcom] alreadyAddRefed_nsFrameLoader GetFrameLoader();

    do you want to just switch to using it and not worry about changing
    nsIFrameLoader?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Also, the duplication here with canvas drawWindow code in terms of flushing is a bit unfortunate; perhaps a followup bug on consolidating that?

Comment 21

[Joe Drew (not getting mail)]

Attachment: don't change nsIFrameLoader, use nsFrameLoader instead.

Comment 22

[Boris Zbarsky [:bzbarsky]]

>+    nsIPresShell* presShell = presContext->PresShell();
>+    NS_ENSURE_TRUE(presShell, NS_ERROR_FAILURE);

If you think it might be null, use GetPresShell().  I see no reason it would be null here.

>+    nsLayoutUtils::sDisableGetUsedXAssertions = oldDisableValue || flush;

Should be !flush, shouldn't it?

>+void DocumentRendererParent::DrawToCanvas(PRUint32 aWidth, PRUint32 aHeight,
>+				    const nsCString& aData)

Fix indent?  In fact, you have all sorts of indentation issues all over DocumentRendererChild/DocumentRendererParent.  Did tabs sneak in?

>+    nsCOMPtr<nsFrameLoader> frameloader =
>+			getter_AddRefs(frameloaderowner->GetFrameLoader());

  nsRefPtr<nsFrameLoader> frameLoader = frameloaderowner->GetFrameLoader();

Also, perhaps loaderOwner would be easier on the eyes?

> +    if (NS_FAILED(frameloader->GetChildProcess(&child)) || !child)

Why not just have GetChildProcess return the pointer instead of nsresult and out param, especially if we plan to ignore the nsresult anyway.

> nsCanvasRenderingContext2D::AsyncDrawXULElement

This is duplicating a lot of DrawWindow code.  Can we refactor (as a followup if desired)?

I have a hard time believing the imgLoader and imgRequest include changes are actually part of this patch.  Are they?

I would like to see a test passing {} as the |elem| argument of this method; in particular because nsIFrameLoaderOwner _is_ scriptable and hence can be implemented by random JS objects.  I would hope the existence of that noscript/notxpcom method means xpconnect won't allow such implementations, but need to test, please.

r=bzbarsky with those nits.

Comment 23

[Joe Drew (not getting mail)]

(In reply to comment #22)
> >+    nsIPresShell* presShell = presContext->PresShell();
> >+    NS_ENSURE_TRUE(presShell, NS_ERROR_FAILURE);
> 
> If you think it might be null, use GetPresShell().  I see no reason it would be
> null here.

That is straight c&p from DrawWindow() code; OTOH NS_ENSURE_TRUE has no place in a function returning bool, so let's just strike that.

> Fix indent?  In fact, you have all sorts of indentation issues all over
> DocumentRendererChild/DocumentRendererParent.  Did tabs sneak in?

Yes. Sneaky, sneaky.
 
> > +    if (NS_FAILED(frameloader->GetChildProcess(&child)) || !child)
> 
> Why not just have GetChildProcess return the pointer instead of nsresult and
> out param, especially if we plan to ignore the nsresult anyway.

An artefact of moving this to & from being an IDL-defined function. Fixed.
 
> > nsCanvasRenderingContext2D::AsyncDrawXULElement
> 
> This is duplicating a lot of DrawWindow code.  Can we refactor (as a followup
> if desired)?

I'm not sure how much we can do, but we should definitely investigate it. Filed bug 525046.

> I have a hard time believing the imgLoader and imgRequest include changes are
> actually part of this patch.  Are they?

No, unrelated stuff I thought I had already put into a separate mq patch.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> has no place in a function returning bool

Er... right.  Same for the !presContet check before that!

> Yes. Sneaky, sneaky.

Please to be adding modelines as needed?  ;)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Oh, and are we handling allocation failure for the image surface ok here?

Comment 26

[Joe Drew (not getting mail)]

OK. Pushed this:
http://hg.mozilla.org/projects/electrolysis/rev/9e1daa4eaa09
and then pushed a non-MOZ_IPC build fix:
http://hg.mozilla.org/projects/electrolysis/rev/873798f6dcdc
and then pushed another fix that I missed:
http://hg.mozilla.org/projects/electrolysis/rev/4052b7e616de

Comment 27

[Joe Drew (not getting mail)]

Had to back out - WebGL's windows code doesn't compile with -DUNICODE because it assumes that the Windows functions it calls are the ASCII versions. Gonna file a bug on that.

Comment 28

[Joe Drew (not getting mail)]

Repushed, with bsmedberg's ASCII-windows fix: http://hg.mozilla.org/projects/electrolysis/rev/f0f4cb0fbb33