405982 - Using png encoder with invalid encoder options throws exception when it used to ignore

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Arun]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1

[Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIDOMHTMLCanvasElement.toDataURL]"  nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)"  location: "JS frame :: "  data: no]

Reproducible: Always

Steps to Reproduce:
I am working for extension compatibility in Firefox 3.0b1. The extension has a feature similar to Save As Image (https://addons.mozilla.org/en-US/firefox/addon/3408). The following is the test case.
1. Download above extension.
2. Make change in maxVersion in install.rdf
3. Install the extension.
4. visit any website.
5. right click and click 'Save Page As Image' (Preview window will be opened)
6. select PNG as output format
7. click 'Save'

This should have opened 'Save As' dialog.

NOTE: If you select JPEG as out format then you will get 'Save As' dialog.
Actual Results:  
[Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIDOMHTMLCanvasElement.toDataURL]"  nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)"  location: "JS frame :: "  data: no]

Expected Results:  
toDataURL should have returned base64 string of PNG image.

Comment 1

[Dave Townsend [:mossop]]

This is working fine in one of my extensions. Could you perhaps create a small testcase demonstrating the problem, maybe even just quoting the toDataURL call would be enough, or try on a newer nightly of Firefox 3.

Comment 2

[Arun]

Attachment: testcase extension has problem with toDataURL

1. Install the extension.
2. Access chrome://testcase405982/content/testcase.html
3. Click 'JPEG' button, should open 'Save As' dialog. (Works fine with 2.0.0.* and 3.0.*)
4. Click 'PNG' button, should open 'Save As' dialog. (Throws exception in 3.0b1, 3.0b2pre nightly(29-Nov-2007)).

Comment 3

[Arun]

(In reply to comment #1)
> This is working fine in one of my extensions. Could you perhaps create a small
> testcase demonstrating the problem, maybe even just quoting the toDataURL call
> would be enough, or try on a newer nightly of Firefox 3.
> 

I have added an attachment to this bug, which has the problem with nightly.
Following is the code to reproduce.

function snapPage(format) {
    var canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "html:canvas");
    var context = canvas.getContext("2d");
    var self ={};
    var cwin = window;
    var db = cwin.document.body;
    var root = cwin.document.documentElement;
    if (root != "[object XPCNativeWrapper [object HTMLHtmlElement]]" && root != "[object HTMLHtmlElement]") {
        throw 1;
    }
    if (root.offsetHeight > root.scrollHeight) {
        self.height = root.offsetHeight;
    } else {
        self.height = root.scrollHeight;
    }

    if (root.offsetWidth > root.scrollWidth) {
        self.width = root.offsetWidth;
    } else {
        self.width = root.scrollWidth;
    }
    if (self.height > 16384) {
        self.height = 16384;
    }
    self.left=0;
    self.top=0;
    canvas.width = self.width;
    canvas.height = self.height;
    context.drawWindow(cwin, self.left, self.top, self.width, self.height, "rgb(255,255,255)");
    internalSave(canvas.toDataURL("image/"+format, "quality=100"), null, "snap-page."+format, null, "image/"+format, true, null, null, null, false);
}

Comment 4

[Dave Townsend [:mossop]]

The problem here is the use of "quality=100". Before bug 375741 the PNG encoder silently dropped any invalid options, it now fails when it sees any invalid options. The JPEG encode currently ignores invalid arguments, I guess it would make sense for the two to be consistent (I generally prefer failing for invalid arguments though).

There is also a bug in the canvas toDataURL implementation in that it ignores any failures on initialising the encoder which leads to the odd NS_BASE_STREAM_CLOSED rather than the NS_ERROR_INVALID_ARG that would normally appear.

Comment 5

[Justin Dolske [:Dolske]]

Yeah, I'd call this bug INVALID. There shouldn't be any "quality" param to the PNG encoder. See modules/libpr0n/public/imgIEncoder.idl.

Comment 6

[Arun]

NS_ERROR_INVALID_ARG would be better error to throw in such case.

Comment 7

[Dave Townsend [:mossop]]

Attachment: properly check for errors

So here is what I think we should do. This correctly returns any errors from the initialisation of the image encoder, this means that the exception thrown out in the testcase is the more sensible NS_ERROR_INVALID_ARGS. I've also made the jpeg encoder consistent with the png one by failing on any invalid options passed.

This is a difference from the branch, but I agree with dolske that we should be failing on invalid options.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 292334 [details] [diff] [review]
properly check for errors

Looks good to me. :)

/driveby

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 292334 [details] [diff] [review]
properly check for errors

This is a small patch that brings some additional consistency to the platform as well as returning errors more likely to be recognisable. It includes a unit test and I would estimate the risk as extremely low.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 292334 [details] [diff] [review]
properly check for errors

a=beltzner for 1.9

Comment 11

[Dave Townsend [:mossop]]

Checking in content/canvas/src/nsCanvasRenderingContext2D.cpp;
/cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v  <--  nsCanvasRenderingContext2D.cpp
new revision: 1.109; previous revision: 1.108
done
Checking in content/canvas/test/Makefile.in;
/cvsroot/mozilla/content/canvas/test/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/content/canvas/test/test_bug405982.html,v
done
Checking in content/canvas/test/test_bug405982.html;
/cvsroot/mozilla/content/canvas/test/test_bug405982.html,v  <--  test_bug405982.html
initial revision: 1.1
done
Checking in modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp;
/cvsroot/mozilla/modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp,v  <--  nsJPEGEncoder.cpp
new revision: 1.6; previous revision: 1.5
done