Closed Bug 1171614 Opened 9 years ago Closed 8 years ago

Inspector doesn't respect "devtools.inspector.imagePreviewTooltipSize" for base64 images

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Version:
40 Branch
Type:
defect

Tracking

(firefox50 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- verified

People

(Reporter: arni2033, Assigned: Kwan, Mentored)

References

Details

(Whiteboard: [lang=js][btpp-backlog])

Attachments

(2 files, 1 obsolete file)

Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.
58 bytes, text/x-review-board-request
Details
Bug 1171614 - Make server inspector actor only return data-URIs as-is if they are smaller than maxDim.
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

Inspect testcase: https://dl.dropboxusercontent.com/s/sa0vle34cvfsuyj/data%20image%20png.html?dl=0
(You may need to reduce the height of the DevTools)


Actual results:

Screenshot - normal pic: https://dl.dropboxusercontent.com/s/gm58zzp1utf3xg6/data%20image%20png%201%20-%20normal.png?dl=0
Screenshot - base64 pic: https://dl.dropboxusercontent.com/s/deaek6b3ubz8s6w/data%20image%20png%202%20-%20data.png?dl=0
Component: General → Developer Tools: Inspector
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(pbrosset)
That's correct, the imageToImageData server-side function [1] we use to transfer the image data to the inspector so it can be shown in the tooltip has a separate code path for when the image is already a data-uri [2]. And in this code path, it doesn't use the resizeRatio.

This bug should be fixed after bug 1192536 lands, otherwise merging will be very painful.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#3929-3972
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#3951
Mentor: pbrosset
Depends on: 1192536
Flags: needinfo?(pbrosset)
Whiteboard: [lang=js]
Has STR: --- → yes
Inspector bug triage. Filter on CLIMBING SHOES.

Does not prevent the user from using the tool -> P3.
Priority: -- → P3
See Also: → 1266448
Whiteboard: [lang=js] → [lang=js][btpp-backlog]
This seems to have been fixed by bug 1266448, which made it so the client now also does resizing.  Is that enough to close this bug?  Or should the server still resize the data: image?
Flags: needinfo?(pbrosset)
Transferring needinfo to Julian.
Flags: needinfo?(pbrosset) → needinfo?(jdescottes)
The bug is no longer visible for users, but I still think we should fix the method mentioned by Patrick: see https://hg.mozilla.org/mozilla-central/file/b69a5bbb5e40bd426e35222baa600b481e50d265/devtools/server/actors/inspector.js#l3034

In my opinion, the optimization done here is not worth keeping. We are saving one canvas::drawImage, but this is a cheap operation and it is not on a critical path. And that's at the expense of having a consistent behavior for the method.

The image preview tooltip is already covered by mochitests (eg browser_markup_image_tooltip.js), so this bug should be fixed simply by removing the if statement I linked above. 

Optionally, it could be nice to extend the test test_inspector_getImageData.html in order to check that the images are actually resized/not resized. This test is also missing the data-uri case, which would need to be added.

Ian: are you interested in taking this bug?
Flags: needinfo?(jdescottes) → needinfo?(moz-ian)
(In reply to Julian Descottes [:jdescottes] from comment #5)
> The bug is no longer visible for users, but I still think we should fix the
> method mentioned by Patrick: see
> https://hg.mozilla.org/mozilla-central/file/
> b69a5bbb5e40bd426e35222baa600b481e50d265/devtools/server/actors/inspector.
> js#l3034
> 
> In my opinion, the optimization done here is not worth keeping. We are
> saving one canvas::drawImage, but this is a cheap operation and it is not on
> a critical path. And that's at the expense of having a consistent behavior
> for the method.
> 
> The image preview tooltip is already covered by mochitests (eg
> browser_markup_image_tooltip.js), so this bug should be fixed simply by
> removing the if statement I linked above. 
> 
> Optionally, it could be nice to extend the test
> test_inspector_getImageData.html in order to check that the images are
> actually resized/not resized. This test is also missing the data-uri case,
> which would need to be added.
> 
> Ian: are you interested in taking this bug?
Yeah sure, I'd already made a change that kept the optimisation, (adding a resizeRatio == 1 check to the data if), but happy to do it this way as well.
You make a good point about actually checking the resizing, tried making a test that'd fail currently with a large data-uri image, only to have it pass, then realised the server currently lies about resizing in that case.
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Flags: needinfo?(moz-ian)
Attachment #8767324 - Flags: review?(pbrosset) → review?(jdescottes)
Attachment #8767325 - Flags: review?(pbrosset) → review?(jdescottes)
This looks pretty good to me, but since I've been drowning in reviews all week, and since Julian has already started looking into the details here, I'd prefer him to do the final review.
Comment on attachment 8767324 [details]
Bug 1171614 - Remove the special path for data-URI images from the server inspector actor

https://reviewboard.mozilla.org/r/61860/#review59046

Looks good to me thanks!
Attachment #8767324 - Flags: review?(jdescottes) → review+
Comment on attachment 8767325 [details]
Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.

https://reviewboard.mozilla.org/r/61862/#review58636

Looks good thanks! 

r+ with my comments addressed and a green push to try (can be triggered from the mozreview UI, let me know if you need help for this!).

::: devtools/server/tests/mochitest/inspector_getImageData.html:8
(Diff revision 1)
>  <body>
>    <img class="custom">
> -  <img class="big-horizontal" src="large-image.jpg" style="width:500px;" />
> +  <img class="big-horizontal" src="large-image.jpg" style="width:500px;">
>    <canvas class="big-vertical" style="width:500px;"></canvas>
> -  <img class="small" src="small-image.gif"></img>
> +  <img class="small" src="small-image.gif">
> +  <img class="data" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAlgAAAJYCAYAAAC%2BZpjcAAAABmJLR0QAAAAAAAD5Q7t%2FAAAACXBIWXMAAAsTAAALEwEAmpwYAAAI20lEQVR42u3YwQkAMAwDsbh0%2F5XdNRqQRvAjHMnMdAAWaJ0rYIdjAgAAgQUAILAAAAQWAAACCwBAYAEACCwAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAEFgAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAILAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQAgsAAAEFgAAAILAEBgAQAgsAAABBYAgMACAEBgAQAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgBAYAEACCwAAIEFAIDAAgAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQCAwAIAEFgAAAILAACBBQAgsAAABBYAgMACAEBgAQAILAAAgQUAgMACABBYAAACCwAAgQUAILAAAAQWAAACCwBAYAEACCwAAIEFAIDAAgAQWAAAAgsAAIEFACCwAAAEFgAAAgsAQGABAAgsAACBBQCAwAIAEFgAAAILAACBBQAgsAAABBYAAAILAEBgAQAILAAABBYAgMACABBYAAACCwAAgQUAILAAAAQWAAACCwBAYAEACCwAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAEFgAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAILAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEACCwAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAQWAAAAgsAQGABACCwAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAgsAAABBYAgMACAEBgAQAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgBAYAEACCwAAIEFAIDAAgAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQCAwAIAEFgAAAILAEBgAQAgsAAABBYAgMACAEBgAQAILAAAgQUAgMACABBYAAACCwAAgQUAILAAAAQWAIDAAgBAYAEACCwAAIEFAIDAAgAQWAAAAgsAAIEFACCwAAAEFgAAAgsAQGABAAgsAACBBQCAwAIAEFgAAAILAACBBQAgsAAABBYAAAILAEBgAQAILAAABBYAgMACABBYAAACCwAAgQUAILAAAAQWAAACCwBAYAEACCwAAAQWAIDAAgAQWAAAAssEAAACCwBAYAEACCwAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAQWAAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAgsAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgBAYAEACCwAAIEFACCwAAAQWAAAf7ltrQCskMQIwAo%2BWAAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAILAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQAgsAAAEFgAAAILAEBgAQAgsAAABBYAgMACAEBgAQAILAAAgQUAgMACABBYAAACCwBAYAEAILAAAAQWAIDAAgBAYAEACCwAAIEFAIDAAgAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQCAwAIAEFgAAAILAACBBQAgsAAABBYAgMACAEBgAQAILAAAgQUAgMACABBYAAACCwAAgQUAILAAAAQWAAACCwBAYAEACCwAAIEFAIDAAgAQWAAAAgsAAIEFACCwAAAEFgAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAILAACBBQAgsAAABBYAAAILAEBgAQAILAAABBYAgMACABBYAAACCwAAgQUAILAAAAQWAAACCwBAYAEACCwAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAEFgAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAILAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAQWAAAAgsAQGABACCwAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAgsAAABBYAgMACAEBgAQAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgBAYAEACCwAAIEFAIDAAgAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQCAwAIAEFgAAAILAACBBQAgsAAABBYAgMACAEBgAQAILAAAgQUAgMACABBYAAACCwAAgQUAILAAAAQWAIDAMgEAgMACABBYAAACCwAAgQUAILAAAAQWAAACCwBAYAEACCwAAAQWAIDAAgAQWAAAAgsAAIEFACCwAAAEFgAAAgsAQGABAAgsAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAABBYAAAILAEBgAQAILAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEACCwAAAQWAIDAAgAQWAAACCwAAIEFACCwAAAQWAAAAgsAQGABACCwAAAEFgCAwAIAEFgAAAgsAACBBQAgsAAAEFgAAAILAEBgAQAgsAAABBYAgMACABBYAAAILAAAgQUAILAAABBYAAACCwBAYAEAILAAAAQWAIDAAgBAYAEACCwAAIEFACCwAAAQWAAAAgsAQGABACCwAAAEFgCAwAIAQGABAAgsAACBBQCAwAIAEFgAAAILAEBgAQAgsAAAPvMAew8Lrgcxl5QAAAAASUVORK5CYII%3D">

Can we use a smaller base64 png? For instance 

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABwAAAAcCAYAAAByDd+UAAAAJklEQVRIie3NMREAAAgAoe9fWls4eAzMVM0xoVAoFAqFQqFQ+C9chp4NHvu+4Q4AAAAASUVORK5CYII=

is a 2x2 black square, which is enough for the purpose of this test.

::: devtools/server/tests/mochitest/test_inspector_getImageData.html:135
(Diff revision 1)
>  /**
> + * Checks if the server told the truth about resizing the image
> + */
> +function testResizing(imageData, str) {
> +  let img = document.createElement('img');
> +  img.src = str;

I think could cause intermittents, could you attach the "load" event listener first and then set the "src" ?
Attachment #8767325 - Flags: review?(jdescottes) → review+
https://reviewboard.mozilla.org/r/61862/#review58636

> Can we use a smaller base64 png? For instance 
> 
> data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABwAAAAcCAYAAAByDd+UAAAAJklEQVRIie3NMREAAAgAoe9fWls4eAzMVM0xoVAoFAqFQqFQ+C9chp4NHvu+4Q4AAAAASUVORK5CYII=
> 
> is a 2x2 black square, which is enough for the purpose of this test.

Done.  Originally chose the size with the maxDim pref value in mind, but oviously we manually set the maxDim anyway, so that doesn't matter. Adjusted that to 14 (The image is 28x28 when I loaded it, guessing 2x2 was a typo)

> I think could cause intermittents, could you attach the "load" event listener first and then set the "src" ?

Oh, duh, of course. Done.
Also changed the createElement() string to use double quotes.
Comment on attachment 8767324 [details]
Bug 1171614 - Remove the special path for data-URI images from the server inspector actor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61860/diff/1-2/
Attachment #8767324 - Flags: review+ → review?(pbrosset)
Attachment #8767325 - Flags: review+ → review?(pbrosset)
Comment on attachment 8767325 [details]
Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61862/diff/1-2/
Comment on attachment 8767324 [details]
Bug 1171614 - Remove the special path for data-URI images from the server inspector actor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61860/diff/2-3/
Attachment #8767324 - Flags: review?(pbrosset)
Attachment #8767325 - Flags: review?(pbrosset)
Comment on attachment 8767325 [details]
Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61862/diff/2-3/
Sorry for the reviewspam earlier Patrick, forgot to update the commit messages before the review push.

Thanks for the review Julian.

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ca357f2ae3
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/52434d177be3
Remove the special path for data-URI images from the server inspector actor r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/0dafaad484d4
Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing r=jdescottes
Keywords: checkin-needed
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b9cc2dcbf4363c8049183110fe79e026fae87ecb since one of this 2 changes seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=10330926&repo=fx-team 

can you take a look since i'm not sure which of this 2 changes caused this regressions
Flags: needinfo?(moz-ian)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/1a38e8bf11b6
Backed out changeset 0dafaad484d4
What is failing: in the inspector when viewing an element with a background-image you can right-click the image URL in the rule pane and hit "Copy Image Data-URL".  A test for this on a data-URI background image is now failing.

Why it is failing: the test expects to get back the exact same data URI, but we just got rid of the code path that special case returns data URIs as is, and there's no chance the re-encode will turn out to be the same anyway because the example is a GIF, and we encode as PNG.

How can it be fixed: either change the tested data-URI to the PNG that we're getting back, or keep the GIF, but change the expected return value to be the PNG.
Flags: needinfo?(moz-ian)
Review commit: https://reviewboard.mozilla.org/r/62818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62818/
Attachment #8767325 - Attachment description: Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing → Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.
Attachment #8768756 - Flags: review?(jdescottes)
Comment on attachment 8767325 [details]
Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61862/diff/3-4/
Attachment #8767324 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/62818/#review59718

Looks good, thanks for updating the patch!

::: devtools/server/actors/inspector.js:3035
(Diff revision 1)
>    // Extract the image data
>    let imageData;
>    // The image may already be a data-uri, in which case, save ourselves the
> -  // trouble of converting via the canvas.drawImage.toDataURL method
> -  if (isImg && node.src.startsWith("data:")) {
> +  // trouble of converting via the canvas.drawImage.toDataURL method, but only
> +  // if the image doesn't need resizing
> +  if (isImg && node.src.startsWith("data:") && resizeRatio == 1) {

nit: the method is later using resizeRatio !== 1, so let's use resizeRatio === 1 for consistency here.
Comment on attachment 8768756 [details]
Bug 1171614 - Make server inspector actor only return data-URIs as-is if they are smaller than maxDim.

https://reviewboard.mozilla.org/r/62818/#review59720
Attachment #8768756 - Flags: review?(jdescottes) → review+
Comment on attachment 8768756 [details]
Bug 1171614 - Make server inspector actor only return data-URIs as-is if they are smaller than maxDim.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62818/diff/1-2/
Comment on attachment 8767325 [details]
Bug 1171614 - Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61862/diff/4-5/
https://reviewboard.mozilla.org/r/62818/#review59718

> nit: the method is later using resizeRatio !== 1, so let's use resizeRatio === 1 for consistency here.

Done.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/1988a6e4b5f2
Make server inspector actor only return data-URIs as-is if they are smaller than maxDim. r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/e2ee6bee61af
Add a test for data-URI images to test_inspector_getImageData.html, and check that the server is being truthful about resizing. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1988a6e4b5f2
https://hg.mozilla.org/mozilla-central/rev/e2ee6bee61af
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I have successfully reproduce this bug on firefox nightly 41.0a1 (2015-06-04)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0

I found this fix on latest nightly 51.0a1 (2016-08-02)

Mozilla/5.0 (Windows NT 6.1; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID : 20160802030437

[bugday-20160803]
Reproduced this bug in firefox nightly 41.0a1 (2015-06-04) as comment 0  with ubuntu 16.04 (64 bit)

Verified this bug as fixed with latest firefox nightly 50.0a1 (Build ID: 20160801030227)
Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
QA Whiteboard: [bugday-20160803]
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: