Bug 60770 – [Qt] QtWebKit bridge assignToHTMLImageElement() results in invalid Image for WebGLRenderingContext

• Component: WebKit Qt
• Status: RESOLVED WONTFIX

[Andrew Wason]

Created attachment 93451 [details]
sample Qt app that uses gl.texImage2D on an image from assignToHTMLImageElement()

I'm trying to use an Image with WebGL populated via QtWebKit bridge assignToHTMLImageElement(). WebGL gl.texImage2D() validates the passed image, and since images created this way have no URL, they are treated as invalid. This is a similar issue to bug #59972

WebCore::WebGLRenderingContext::texImage2D() calls WebCore::WebGLRenderingContext::validateHTMLImageElement() which checks the image url:


Source/WebCore/html/canvas/WebGLRenderingContext.cpp:validateHTMLImageElement():

    const KURL& url = image->cachedImage()->response().url();
    if (url.isNull() || url.isEmpty() || !url.isValid()) {
        m_context->synthesizeGLError(GraphicsContext3D::INVALID_VALUE);
        return false;
    }

Attached sample application reproduces the bug.

Possibly Images populated via assignToHTMLImageElement() could have a placeholder qrc:/ or data: url assigned? There are probably other places in WebKit that will treat images with no url as invalid.

Or perhaps the bridge intermediate JS object could be extended to have toImageData() or toArrayBuffer() methods to access the image pixels, these would both be useable with gl.texImage2D()

[Andrew Wason]

Created attachment 93505 [details]
set CachedImage response URL

Attached patch fixes this by allowing an optional URL to be passed to CachedImage constructor, and this is set on the ResourceResponse. The Qt bridge is changed to construct the image with a qrc:/assignedImage url.

An alternative would be to change the test in WebGLRenderingContext::validateHTMLImageElement, but I'm not sure what a valid test would be.

[Benjamin Poulain]

Comment on attachment 93505 [details]
set CachedImage response URL

View in context: https://bugs.webkit.org/attachment.cgi?id=93505&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Why is there not no test?

[Benjamin Poulain]

The patch looks more like a big workaround.

If an origin has to be set, I would rather see the origin of the frame.

The code of WebGLRenderingContext.cpp might also be bogus. Is there no way to reproduce the same issue by manipulating the image directly or with Canvas?

[Andrew Wason]

(In reply to comment #3)
> The patch looks more like a big workaround.
> 
> If an origin has to be set, I would rather see the origin of the frame.

OK, I think using the frame origin would also fix bug #59972

> The code of WebGLRenderingContext.cpp might also be bogus.
> Is there no way to reproduce the same issue by manipulating the image directly or with Canvas?

A similar problem (bug #59972) can be reproduced with Canvas, but this bug is specific to how WebGLRenderingContext is validating images.

WebGLRenderingContext::validateHTMLImageElement() was added to fix bug #58501. I think validateHTMLImageElement() could check HTMLImageElement::complete() and that width/height are non-zero, instead of looking at the response url. This seems to be what WebCore::CanvasRenderingContext2D::drawImage() is doing.

So should I use the frame origin (and add tests), or change WebGLRenderingContext::validateHTMLImageElement()? Setting the origin has the advantage of also fixing bug #59972

Maybe this bug should be fixed by changing validateHTMLImageElement() to check complete/width/height instead of response url, and a separate patch to set the image origin to fix bug #59972?

[Andrew Wason]

Created attachment 94256 [details]
set CachedImage response URL

Set CachedImage response URL to the Images document URL. Implement StillImage::hasSingleSecurityOrigin() to return true. Add tests to check gl.texImage2D does not return an error, and that getImageData on 2D canvas does not throw an exception (bug #59972)

[Antonio Gomes]

Comment on attachment 94256 [details]
set CachedImage response URL

View in context: https://bugs.webkit.org/attachment.cgi?id=94256&action=review

> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:103
> +        imageElement->setCachedImage(new CachedImage(stillImage.get(), document->documentURI()));

Just a side comment: I think new'ing an object like this (and passing ownership) is a bad practice in WebKit.

[Alexis Menard (darktears)]

Comment on attachment 94256 [details]
set CachedImage response URL

View in context: https://bugs.webkit.org/attachment.cgi?id=94256&action=review

>> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:103
>> +        imageElement->setCachedImage(new CachedImage(stillImage.get(), document->documentURI()));
> 
> Just a side comment: I think new'ing an object like this (and passing ownership) is a bad practice in WebKit.

+1

[Andrew Wason]

(In reply to comment #6)
> 
> Just a side comment: I think new'ing an object like this (and passing ownership) is a bad practice in WebKit.

What is the preferred practice?

[Antonio Gomes]

(In reply to comment #8)
> (In reply to comment #6)
> > 
> > Just a side comment: I think new'ing an object like this (and passing ownership) is a bad practice in WebKit.
> 
> What is the preferred practice?

See http://www.webkit.org/coding/RefPtr.html (PassRefPtr section)

[Noam Rosenthal]

Comment on attachment 94256 [details]
set CachedImage response URL

View in context: https://bugs.webkit.org/attachment.cgi?id=94256&action=review

> Source/WebCore/loader/cache/CachedImage.h:42
> +    CachedImage(Image*, const String& url = emptyString());

Does it have to be in the constructor? Seems like something like setUrl() would be more readable, since that what's you've added to the constructor anyway.

> Source/WebKit/qt/tests/hybridPixmap/widget.cpp:33
> +    ui->webView->settings()->setAttribute(QWebSettings::WebGLEnabled, true);

So this API test would fail if built without WebGL?

[Andrew Wason]

Created attachment 99312 [details]
set CachedImage response URL

Add CachedImage::setURL() instead of passing in constructor.

[Andrew Wason]

(In reply to comment #10) 
> Does it have to be in the constructor? Seems like something like setUrl() would be more readable, since that what's you've added to the constructor anyway.

I updated the patch to add CachedImage::setURL

> > Source/WebKit/qt/tests/hybridPixmap/widget.cpp:33
> > +    ui->webView->settings()->setAttribute(QWebSettings::WebGLEnabled, true);
> 
> So this API test would fail if built without WebGL?

No, WebKit/qt/tests/hybridPixmap/test.html skips the WebGL tests if it is disabled. I built with WebGL disabled and the tests pass.

[WebKit Review Bot]

Attachment 99312 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/qt/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/qt/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/qt/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrew Wason]

Created attachment 99315 [details]
set CachedImage response URL

Fixed ChangeLog style issues

[WebKit Review Bot]

Comment on attachment 99315 [details]
set CachedImage response URL

Attachment 99315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8970108

New failing tests:
http/tests/local/fileapi/send-dragged-file.html

[WebKit Review Bot]

Created attachment 99317 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Noam Rosenthal]

(In reply to comment #14)
> Created an attachment (id=99315) [details]
> set CachedImage response URL
> 
> Fixed ChangeLog style issues

I have no problem with this patch from the Qt bridge aspect. I don't feel comfortable reviewing changes to CachedImage, maybe someone else can :)

[Eric Seidel (no email)]

Ping?  Should this be r-'d?  It's been 8 months...

[Noam Rosenthal]

Andrew, we should probably make WebGL on Qt a bit more stable before we enable this.
If by then you still care about it, we can look at this again.

[Andrew Wason]

(In reply to comment #19)
> Andrew, we should probably make WebGL on Qt a bit more stable before we enable this.
> If by then you still care about it, we can look at this again.

OK. I've been working around this by getting ImageData from the Qt bridge to use with WebGL (instead of Image) - which avoids the validation check.

[Brent Fulgham]

The QT port is no longer part of WebKit.