Closed Bug 505847 Opened 11 years ago Closed 10 years ago

Electrolysis + tilebrowser (async .drawWindow)

Categories

(Core :: IPC, defect)

Other Branch
x86
Windows NT
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: joe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

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.
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?
Blocks: 516521
Depends on: 520666
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.
Attachment #404722 - Flags: review?(jones.chris.g)
I don't see PDocumentRenderer.ipdl in the patch, although it's referenced in a few places.  Need to |hg add| it maybe?
Attached patch add missing files (obsolete) — Splinter Review
Forgot to hg add all the files I added - whoops.
Attachment #404722 - Attachment is obsolete: true
Attachment #404877 - Flags: review?(jones.chris.g)
Attachment #404722 - Flags: review?(jones.chris.g)
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.
Attachment #404877 - Flags: review?(jones.chris.g) → review?(benjamin)
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 <joe@drew.ca>.
>+ * 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
Attachment #404877 - Flags: review?(benjamin) → review+
> static helper function which can parse colors

That depends on how restricted your color string is.  How restricted is it in this case?
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.
(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.)
Attached patch interdiff of changes (obsolete) — Splinter Review
This is an interdiff for the changes I made to fulfil bsmedberg's review comments.
Comment on attachment 406110 [details] [diff] [review]
interdiff of changes

ignore: doesn't compile and isn't complete.
Attachment #406110 - Attachment is obsolete: true
No longer depends on: 520666
Attached patch address review comments (obsolete) — Splinter Review
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.
Attachment #404877 - Attachment is obsolete: true
Attachment #406130 - Flags: review?(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!
Blocks: 522335
Attachment #406130 - Flags: review?(bzbarsky) → review-
Attached patch address reivew comments, again (obsolete) — Splinter Review
This patch adds childProcess as a noscript attribute to nsIFrameLoader, and properly uses nsIFrameLoader and nsIFrameLoaderOwner (and nsCOMPtr!!!) to get the PIFrameEmbeddingParent*.
Attachment #406130 - Attachment is obsolete: true
Attachment #407137 - Flags: review?(bzbarsky)
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.
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.
Blocks: 523472
Joe, your patch bitrotted in two places --- I have an updated local version if you'd like it.
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?
Also, the duplication here with canvas drawWindow code in terms of flushing is a bit unfortunate; perhaps a followup bug on consolidating that?
Attachment #407137 - Attachment is obsolete: true
Attachment #408090 - Attachment is obsolete: true
Attachment #408504 - Flags: review?(bzbarsky)
Attachment #407137 - Flags: review?(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.
Attachment #408504 - Flags: review?(bzbarsky) → review+
Blocks: 525046
(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.
No longer blocks: 525046
Component: DOM: Mozilla Extensions → IPC
QA Contact: general → ipc
> 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?  ;)
Oh, and are we handling allocation failure for the image surface ok here?
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 525106
Repushed, with bsmedberg's ASCII-windows fix: http://hg.mozilla.org/projects/electrolysis/rev/f0f4cb0fbb33
Depends on: 525677
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Version: unspecified → Other Branch
You need to log in before you can comment on or make changes to this bug.