Closed Bug 405982 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: findarunhere, Assigned: mossop)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
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.
Component: Extension Compatibility → Layout: Canvas
Product: Firefox → Core
QA Contact: extension.compatibility → layout.canvas
Version: unspecified → Trunk
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)).
(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);
}
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.

Status: UNCONFIRMED → NEW
Component: Layout: Canvas → ImageLib
Ever confirmed: true
Keywords: regression
OS: Linux → All
QA Contact: layout.canvas → imagelib
Hardware: PC → All
Summary: calling canvas.toDataURL with mime type image/png throws exception → Using png encoder with invalid encoder options throws exception when it used to ignore
Blocks: 375741
Yeah, I'd call this bug INVALID. There shouldn't be any "quality" param to the PNG encoder. See modules/libpr0n/public/imgIEncoder.idl.
NS_ERROR_INVALID_ARG would be better error to throw in such case.
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.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #292334 - Flags: superreview?
Attachment #292334 - Flags: review?
Attachment #292334 - Flags: superreview?(pavlov)
Attachment #292334 - Flags: superreview?
Attachment #292334 - Flags: review?(pavlov)
Attachment #292334 - Flags: review?
Comment on attachment 292334 [details] [diff] [review]
properly check for errors

Looks good to me. :)

/driveby
Attachment #292334 - Flags: review+
Whiteboard: [has patch]
Attachment #292334 - Flags: superreview?(pavlov)
Attachment #292334 - Flags: superreview+
Attachment #292334 - Flags: review?(pavlov)
Attachment #292334 - Flags: review+
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.
Attachment #292334 - Flags: approval1.9?
Comment on attachment 292334 [details] [diff] [review]
properly check for errors

a=beltzner for 1.9
Attachment #292334 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla1.9 M11
You need to log in before you can comment on or make changes to this bug.