Last Comment Bug 659282 - gl.texImage2D() fails when loading images from a chrome URI instead of http
: gl.texImage2D() fails when loading images from a chrome URI instead of http
Status: NEW
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-24 05:25 PDT by Victor Porof [:vporof][:vp]
Modified: 2013-06-26 00:47 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
do not check read access if context is trusted for read (priveleged) (1.09 KB, patch)
2011-05-24 07:14 PDT, Cedric Vivier [:cedricv]
no flags Details | Diff | Splinter Review
Testcase extension, see browserOverlay.js for example (390.58 KB, application/jar)
2011-05-24 09:33 PDT, Victor Porof [:vporof][:vp]
no flags Details

Description Victor Porof [:vporof][:vp] 2011-05-24 05:25:11 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 Gecko/20100101 Firefox/4.0.1

gl.texImage2D() fails when loading images from a chrome:// URI instead of http:// (in which case it works like a charm). The WebGL context is created fine and the image is loaded without issues (regardless if its dimensions are power of two or not), but when passing it as a parameter to gl.texImage2D(), it throws following error:

uncaught exception: [Exception... "Failure arg 5 [nsIDOMWebGLRenderingContext.texImage2D]" nsresult: "0x80004005 (NS_ERROR_FAILURE)"

The call causing this error is a basic
gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, image);
where the fifth argument is the image loaded from a chrome:// resource scheme.

This is probably due to the cross-domain check that does not handle chrome://

Reproducible: Always

Steps to Reproduce:
1.
// create the image
texture.image = new Image();
texture.image.src = textureSource; // this is chrome URI

2.
//in the onload, bind the texture and use texImage2D()
// ...
image.onload = function() {
  gl.bindTexture(gl.TEXTURE_2D, texture);
  gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, image);
}

Actual Results:  
Texture is prohibited to use the data from the image and an error is thrown.

Expected Results:  
Texture should have been created fine using the data from the image.

This happens only when passing images loaded from chrome:// in a Firefox extension.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-05-24 05:53:16 PDT
marking as new.

Victor, you might want to attach a link to a broken version of Tilt that shows this API call failing.
Comment 2 Victor Porof [:vporof][:vp] 2011-05-24 06:49:39 PDT
Ok, Rob.
Lemme first make a Git repo for it.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-05-24 07:09:23 PDT
> This is probably due to the cross-domain check that does not handle chrome://

"Does not handle" in what sense?  Where is the code above running, exactly?
Comment 4 Cedric Vivier [:cedricv] 2011-05-24 07:14:22 PDT
Created attachment 534756 [details] [diff] [review]
do not check read access if context is trusted for read (priveleged)
Comment 5 Cedric Vivier [:cedricv] 2011-05-24 07:14:54 PDT
(In reply to comment #3)
> > This is probably due to the cross-domain check that does not handle chrome://
> "Does not handle" in what sense?  Where is the code above running, exactly?

It is running in a privileged/chrome context (extension).
I believe attached patch might fix it (untested, Victor could you try if it works for you?).
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-05-24 08:12:08 PDT
> It is running in a privileged/chrome context (extension).

But the _canvas_ is not privileged?

In any case, that security check can't lead to an NS_ERROR_FAILURE.  It'll just mark the canvas write-only, no?

NS_ERROR_FAILURE means we have no image surface at all or something.

I'd really appreciate a minimalish testcase (in extension form, presumably) that shows whatever the problem is so I don't have to try to guess what the code is doing.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2011-05-24 08:14:40 PDT
(In reply to comment #6)
> > It is running in a privileged/chrome context (extension).
> 
> But the _canvas_ is not privileged?
> 
> In any case, that security check can't lead to an NS_ERROR_FAILURE.  It'll
> just mark the canvas write-only, no?

Indeed. That changes in bug 656277.

I rather suspect that this exception is thrown from the custom quickstub we have for texImage2D(). Is there a way to get a c++ stack trace for where a JS exception is thrown from?
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-05-24 08:18:47 PDT
I checked the code. The texImage2D() quickstub first tries to interprete the argument as a DOMElement by calling WebGLContext::DOMElementToImageSurface. If WebGLContext::DOMElementToImageSurface returns a failure, then it proceeds to try to interprete it as a ImageData, and if that fails it generates an exception very similar to what you're getting.

So it's looking like WebGLContext::DOMElementToImageSurface returns an error. It would be nice to have a testcase, and step through that function.
Comment 9 Victor Porof [:vporof][:vp] 2011-05-24 08:42:07 PDT
(In reply to comment #8)
> I checked the code. The texImage2D() quickstub first tries to interprete the
> argument as a DOMElement by calling WebGLContext::DOMElementToImageSurface.
> If WebGLContext::DOMElementToImageSurface returns a failure, then it
> proceeds to try to interprete it as a ImageData, and if that fails it
> generates an exception very similar to what you're getting.
> 
> So it's looking like WebGLContext::DOMElementToImageSurface returns an
> error. It would be nice to have a testcase, and step through that function.

I'm writing a simple testcase extension right now.
Comment 10 Victor Porof [:vporof][:vp] 2011-05-24 09:33:09 PDT
Created attachment 534799 [details]
Testcase extension, see browserOverlay.js for example
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-05-24 13:00:53 PDT
So what this extension is doing is:

1)  Creating an untrusted canvas element (by loading an untrusted iframe).
2)  Grabbing the canvas element from that frame.
3)  Trying to paint a chrome:// image to that canvas.

We very definitely do NOT allow that.  I don't think we should start allowing it either, honestly.....

That said, I'd think this should happen with an http:// URI too, since that's not subsumed by the canvas origin either.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2011-05-24 13:28:38 PDT
Boris, this is presumably motivated by Victor's GSOC project on Tilt i.e. WebGL visualization of the DOM in a page. Could you share your thoughts on what needs to be changed in his design to make it acceptable?
Comment 13 Victor Porof [:vporof][:vp] 2011-05-24 13:57:59 PDT
(In reply to comment #5)
> It is running in a privileged/chrome context (extension).
> I believe attached patch might fix it (untested, Victor could you try if it
> works for you?).

Just finished compiling. It doesn't fix it, the same exception is thrown.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-05-24 14:04:22 PDT
Well, the simplest fix would be to use a chrome-principal canvas (e.g. by not setting type="content" on the iframe he's using).  Or is there a good reason you actually want an unpriviliged subframe for the canvas?
Comment 15 Victor Porof [:vporof][:vp] 2011-05-25 05:19:05 PDT
(In reply to comment #14)
> Well, the simplest fix would be to use a chrome-principal canvas (e.g. by
> not setting type="content" on the iframe he's using).  Or is there a good
> reason you actually want an unpriviliged subframe for the canvas?

The
iframe.setAttribute("type", "content");
was removed and the same exception is thrown.
Comment 16 Cedric Vivier [:cedricv] 2011-05-25 07:13:16 PDT
So I've investigated this a bit more.
The issue happens in the quickstub at CustomQS_WebGL:327, where the arg cannot be unwrapped to a IDOMElement, I guess "new Image" in chrome/XUL context gives something else.

As a work around it works by explicitly creating an HTML image :

texture.image = document.createElementNS("http://www.w3.org/1999/xhtml","image");
texture.image.setAttribute("src", chromePath);
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-05-25 07:41:06 PDT
Hmm.  |new Image()| in a chrome context should give you an HTMLImageElement, I'd think...
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2011-05-25 07:49:59 PDT
(In reply to comment #16)
> So I've investigated this a bit more.
> The issue happens in the quickstub at CustomQS_WebGL:327, where the arg
> cannot be unwrapped to a IDOMElement, I guess "new Image" in chrome/XUL
> context gives something else.


For reference, here's this code:

    if (argc > 5 &&
        !JSVAL_IS_PRIMITIVE(argv[5]))
    {
        // implement the variants taking a DOMElement as argv[5]
        GET_UINT32_ARG(argv2, 2);
        GET_UINT32_ARG(argv3, 3);
        GET_UINT32_ARG(argv4, 4);

        nsIDOMElement *elt;
        xpc_qsSelfRef eltRef;
        rv = xpc_qsUnwrapArg<nsIDOMElement>(cx, argv[5], &elt, &eltRef.ptr, &argv[5]);
        if (NS_FAILED(rv)) return JS_FALSE;


Line 327 is the xpc_qsUnwrapArg call here. Let me know if your line numbers are different.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-05-25 07:56:00 PDT
What is argv[5] in your case, exactly?
Comment 20 Cedric Vivier [:cedricv] 2011-05-25 11:04:22 PDT
Sorry, the exception is thrown obviously at line 344 on latest, BadArg 5... which is the message we get in the exception.

So it happens after unwrap - and failed TexImage2D - when trying to interpret it as an ImageData object... which fails because _it is indeed an nsIDomElement_.


So, back inside TexImage2D, let's follow what's going on :

1.  WebGLContextGL.cpp:~3311
    nsLayoutUtils::SurfaceFromElement(imageOrCanvas, flags);
    if (!res.mSurface) {
        return NS_ERROR_FAILURE;

    SurfaceFromElement call fails.


2.  nsLayoutUtils.cpp:~3920
    rv = imgContainer->GetFrame(whichFrame,
                                frameFlags,
                                getter_AddRefs(framesurf));

    Fails.


3.  In RasterImage::GetFrame (RasterImage.cpp:~691)

    if (!CanForciblyDiscard() || mDecoder || mAnim)
      return NS_ERROR_NOT_AVAILABLE;

    The first condition fails, because mDiscardable is false.
    It appears this is here that the code path differs between a chrome image and a non-chrome image, indeed the chrome image is not discardable whereas http images are.


Above check is performed because is WebGL is requesting surfaces with FLAG_SYNC_DECODE to make sure all data is returned before function exits... which seems to require the image to be discardable for some reason (any pointers why?).


What makes chrome images specifically non-discardable lies in imgRequest.cpp:~1069 :

    // We want UI to be as snappy as possible and not to flicker. Disable discarding
    // and decode-on-draw for chrome URLS
    PRBool isChrome = PR_FALSE;
    rv = mURI->SchemeIs("chrome", &isChrome);
    if (NS_SUCCEEDED(rv) && isChrome)
      isDiscardable = doDecodeOnDraw = PR_FALSE;

Which kinda makes sense so it probably doesn't sound wise to change this behavior.
Note that resource:// images are also set non-discardable.



If I'm not completely mistaken, this means that to support WebGL in chrome, we need either to remove the discardable requirement for SYNC_DECODE - if it is indeed necessary -, or make WebGL somehow force (re)loading such content as discardable.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2011-05-25 11:26:14 PDT
ccing imagelib folks to answer those questions.

Cedric for figuring out what's really happening here.  At least now the "http works" part makes sense.  ;)
Comment 22 Joe Drew (not getting mail) 2011-05-25 11:47:22 PDT
OK. The problem here is that we are trying to get an image with flags other than the flags it's currently using. To do that, we need to redecode the image, and we don't allow discarding and redecoding chrome images.

In more detail: WebGL lets you specify UNPACK_PREMULTIPLY_ALPHA and UNPACK_COLORSPACE_CONVERSION (in PixelStore) to values other than that usually used by Gecko. I highly suspect this is what has happened here. If you specify them as UNPACK_PREMULIPLY_ALPHA as false, and UNPACK_COLORSPACE_CONVERSION as BROWSER_DEFAULT_WEBGL, you should not run into this problem.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2011-05-25 11:54:44 PDT
Do we really need to redecode?  Or could we just convert from the one format to the other?

Or is the point that WebGL should get them with the default settings and then convert itself as needed?
Comment 24 Joe Drew (not getting mail) 2011-05-25 12:13:46 PDT
You can go from unpremultiplied to premultiplied, but that's a lossy conversion. You could probably undo colour conversion, too. We didn't bother with any of that, though - we just support turning them on and off at decode time, since it's easier.

My point is that I'm almost certain you don't need to worry about premultiplication/colour correction in this particular application, so turning those knobs such that there's no change necessary from Gecko's point of view will make this Just Work.
Comment 25 Cedric Vivier [:cedricv] 2011-05-25 12:16:59 PDT
(In reply to comment #22)
> If you specify them as UNPACK_PREMULIPLY_ALPHA as false, and
> UNPACK_COLORSPACE_CONVERSION as BROWSER_DEFAULT_WEBGL, you should not run
> into this problem.

Interesting.

These are WebGL defaults though, decodeFlags for the GetFrame call here is FLAG_DECODE_NO_PREMULTIPLY_ALPHA (2) ... but imgContainer's mFrameDecode is FLAG_NONE (0).

Is premultiply alpha the default for imagelib?
Comment 26 Joe Drew (not getting mail) 2011-05-25 12:31:27 PDT
Err, sorry. I misread the WebGL code. You need specifically to call PixelStore with UNPACK_PREMULTIPLY_ALPHA to *true*. Premultiplication is indeed the default for imagelib.

You'll probably also need to change your blend mode, because you'll no longer be using premultiplied images, but that can come later.
Comment 27 Cedric Vivier [:cedricv] 2011-05-25 18:42:49 PDT
Hmm, wouldn't it make sense for imagelib to automatically set mFrameDecodeFlags's NO_PREMULTIPLY_ALPHA when the image/decoder does not (or cannot even) have an alpha channel as in this testcase (jpeg) ?

Special care to work around this limitation would therefore be needed (noticed) only when actually loading an image with an alpha channel (intuitively enough).
Comment 28 adrian.arroyocalle 2013-06-25 05:43:44 PDT
I used XUL Runner 21 and the problem still exists.
Comment 29 Jeff Gilbert [:jgilbert] 2013-06-25 14:12:06 PDT
(In reply to adrian.arroyocalle from comment #28)
> I used XUL Runner 21 and the problem still exists.

Does it still exist if you set UNPACK_PREMULTIPLY_ALPHA to `true`, as from Comment 26?
Comment 30 adrian.arroyocalle 2013-06-26 00:47:56 PDT
Well, I'm using the THREE.JS library for my project so I can't change the that property. Currently I'm loading a FILE:// URI in XUL Runner and works fine. I get the path with the "AChrom" property. I also say that the RESOURCE:// URI fails with the same error than CHROME:// URI.

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