783223 - Saved screenshots are not valid JPEGs

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Attempted screenshot of back button

It appears that bug 761423 caused a regression with the saved screenshots. Any screenshots saved currently contain the data URL rather than the decoded image content.

I have attached a screenshot of the back button captured by Mozmill.

Comment 1

[Szabolcs Hubai (:xabolcs)]

The problem was that the dataURL in saveScreenshot() wasn't converted to any binary format, so it was saved as text.

In the fix I converted to binary inputstream with the help og NetUtil.asyncFetch.
That converts nsIURI / nsIChannel to nsIInputStream.

Comment 2

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v1 on Github

A (dataURL -> (binary) inputStream) conversion was needed. 
Done by NetUtil.asyncFetch + newURI. [1]

As You can see, there was a newURI in takeScreenshot too. [2]
In other words: asyncFetch is the new saveURI. :)

[1]: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/NetUtil.jsm
[2]: https://github.com/mozilla/mozmill/commit/eefed2195e#L3L540

Comment 3

[Szabolcs Hubai (:xabolcs)]

Attachment: output of the patched saveScreenshot

Tested with dataURL in Attachment 652398 [details].

Comment 4

[Jeff Hammel]

Comment on attachment 655468 [details]
Patch v1 on Github

I haven't tested this patch, but it looks good to me unless :whimboo has reservations

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks good to me. The only thing I want to see is that we update our test 'mutt/mutt/tests/js/controller/screenshot.js' and ensure that the file has been saved in the expected format. I think it should be enough to test that the content is different from the dataURL. Also we could could check the failing part by making the dataURL invalid.

Szabolcs, you can run the test via mozmill or mutt. Latter one is preferred to ensure we do not break anything. Just specify mutt/mutt/tests/all-tests.ini as manifest.

Comment 6

[Szabolcs Hubai (:xabolcs)]

Attachment: Mozmill log-file output (debug)

Attached mozmill debug output.
Below is the console output.

> $ mozmill -b /c/Program\ Files/Mozilla\ Aurora/firefox.exe -t mutt/mutt/tests/js/controller/screenshot.js -l out.log --file-level=DEBUG --format=pprint
> TEST-START | d:\apps\othr\moz\mozilla-mozmill-8dd8b11\mutt\mutt\tests\js\controller\screenshot.js | setupModule
> TEST-START | d:\apps\othr\moz\mozilla-mozmill-8dd8b11\mutt\mutt\tests\js\controller\screenshot.js | test
> TEST-PASS | d:\apps\othr\moz\mozilla-mozmill-8dd8b11\mutt\mutt\tests\js\controller\screenshot.js | test
> TEST-START | d:\apps\othr\moz\mozilla-mozmill-8dd8b11\mutt\mutt\tests\js\controller\screenshot.js | testChrome
> TEST-PASS | d:\apps\othr\moz\mozilla-mozmill-8dd8b11\mutt\mutt\tests\js\controller\screenshot.js | testChrome
> RESULTS | Passed: 2
> RESULTS | Failed: 0
> RESULTS | Skipped: 0
> 
>

Comment 7

[Szabolcs Hubai (:xabolcs)]

Attachment: Saved screenshot during mozmill run.

After editing check_screenshot() in 'mutt/mutt/tests/js/controller/screenshot.js' 
not to discard files, I was able to backup this screenshot.

Comment 8

[Szabolcs Hubai (:xabolcs)]

Comment on attachment 655468 [details]
Patch v1 on Github

Obsoleted because converting 'aDataURL' to nsIURI is needless.
NetUtil.asyncFetch accepts string URLs too.

See documentation at MDN! [1]


[1]: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/NetUtil.jsm#asyncFetch%28%29

Comment 9

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Looks good to me. The only thing I want to see is that we update our test
> 'mutt/mutt/tests/js/controller/screenshot.js' and ensure that the file has
> been saved in the expected format. I think it should be enough to test that
> the content is different from the dataURL.


I created an example gist [1] which could convert a local file into a dataURL,
based on brody's work [2].
Reading back the file into loadedDataURL and compare to screenshot.dataURL will do the job, as You wrote.

But where to place this check? 
In check_screenshot() in 'mutt/mutt/tests/js/controller/screenshot.js'?
But then the loading shouldn't be async!

And where to place this file2dataURL converter?


> Also we could could check the
> failing part by making the dataURL invalid.

?!

Where / what is the failing part?
Where would be this checking?

What makes a dataURL invalid? 
After (dataURL = "dataURL invalidated!") dataURL would be invalid?

Would You mind to rephrase this. My mozmill-test fu is near zero. :)

Would You mind to file a followup bug about this check 
(with a detailed description to let it be an another [good first bug] :))?



[1]: https://gist.github.com/3498536
[2]: http://forums.mozillazine.org/viewtopic.php?p=5091285#p5091285

Comment 10

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v3 (patchset compareview at GitHub)

Changes since patch v1:
- callback if asyncFetch() fails
- use "aDataURL" as is in asyncFetch() instead as nsIURI.


The pull request is still #94. [1]



[1]: https://github.com/mozilla/mozmill/pull/94

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 656193 [details]
Patch v3 (patchset compareview at GitHub)

This looks good. I don't wanna give a full review yet, given that we have to work on the test a bit. Comment on that will follow in a bit.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Szabolcs Hubai from comment #9)
> But where to place this check? 
> In check_screenshot() in 'mutt/mutt/tests/js/controller/screenshot.js'?

You can place it in there as a separate function which gets called if a screenshot has been saved to disk.

> But then the loading shouldn't be async!

You will have to make it synchronous right.

> And where to place this file2dataURL converter?

We will only need it here. So just add it to the test module.

> > Also we could could check the
> > failing part by making the dataURL invalid.
> 
> ?!
> 
> Where / what is the failing part?
> Where would be this checking?

We can construct an invalid dataURL and pass it to the save screenshot function. Then we check the returned object for expected values.

> What makes a dataURL invalid? 
> After (dataURL = "dataURL invalidated!") dataURL would be invalid?

Or make use of an invalid specifier (unknown format). Not sure what's necessary. But I would simply see a test when asyncFetch() fails.

> Would You mind to file a followup bug about this check 
> (with a detailed description to let it be an another [good first bug] :))?

I think it fits perfect into this bug because it introduces asyncFetch and we want to have tests for new features.

Comment 13

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #12)
> (In reply to Szabolcs Hubai from comment #9)
> > [...]
> > Where / what is the failing part?
> > Where would be this checking?
> 
> We can construct an invalid dataURL and pass it to the save screenshot
> function. Then we check the returned object for expected values.
> 

Done.
Because this is a known-failing test, I copied check_screenshot()
into the new test, and modified there.


> > What makes a dataURL invalid? 
> > After (dataURL = "dataURL invalidated!") dataURL would be invalid?
> 
> Or make use of an invalid specifier (unknown format). Not sure what's
> necessary. But I would simply see a test when asyncFetch() fails.
> 
> [...]
> 

Testing utils.js:saveScreenshot() through controller.js:savescreenShot()
won't be easy.
Instead a new module in 'mutt/mutt/tests/js/utils/' which directly imports utils.js.
But that saveScreenshot() would hardly fail. Only if something goes wrong
with the OS.
IMHO it would be hard to show You a test when asyncFetch() fails.

Comment 14

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v4 (patchset compareview at GitHub)

(In reply to Szabolcs Hubai from comment #13)
> Done.

Pointing to compare view.
Pull request is still #94. [1]


Requesting f? only, because still not sure about
how to test this correctly.

And a question:
How to share a function (check_screenshot) - to avoid diverging - 
between an expected = pass and an expected = false testModule?




[1]: https://github.com/mozilla/mozmill/pull/94

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 657800 [details]
Patch v4 (patchset compareview at GitHub)

In general it looks fine and I can see that we can land it soon. But f- given my comments on Github.

Comment 16

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v5 (patchset compareview at GitHub)

New patchset at GitHub.
Asking for review.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 664744 [details]
Patch v5 (patchset compareview at GitHub)

I think that looks pretty good. Please get the one expect/assert case sorted out and the nits fixed. With it please ask Jeff for review again. I would like to also hear his feedback. Otherwise good work!

Comment 18

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v6 (patchset compareview at GitHub)

New patchset at GitHub.
Asking for multi-review.

Comment 19

[Szabolcs Hubai (:xabolcs)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #18)
> Created attachment 665169 [details]
> Patch v6 (patchset compareview at GitHub)
> 
> New patchset at GitHub.
> Asking for multi-review.

Jeff, would You mind to comment on a thread about file.exists()?

(Which can be found in an outdated diff, 3rd comment after part 4)

Comment 20

[Jeff Hammel]

(In reply to Szabolcs Hubai (:xabolcs) from comment #19)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #18)
> > Created attachment 665169 [details]
> > Patch v6 (patchset compareview at GitHub)
> > 
> > New patchset at GitHub.
> > Asking for multi-review.
> 
> Jeff, would You mind to comment on a thread about file.exists()?
> 
> (Which can be found in an outdated diff, 3rd comment after part 4)

Mind providing a direct link and/or pasting in bugzilla?

Comment 21

[Szabolcs Hubai (:xabolcs)]

(In reply to Jeff Hammel [:jhammel] from comment #20)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #19)
> > 
> > Jeff, would You mind to comment on a thread about file.exists()?
> > 
> > (Which can be found in an outdated diff, 3rd comment after part 4)
> 
> Mind providing a direct link and/or pasting in bugzilla?


It's [1] on GitHub, but it's hidden by default, so not easy to find.
So putting here.

The talk is about the last line:
>+  expect.ok(file.exists(), "DataURL '" + file.path + "' has been saved.");



>+
>+  // Save screenshot to disk and wait for ansynchronous action to be completed
>+  var filename = utils.saveScreenshot(smile5x5DataURL, name, sync);
>+  utils.waitFor(function () {
>+    return ready;
>+  }, "Screenshot '" + filename + "' has been saved.");
>+
>+  expect.ok(!failure, "No failure while saving dataURL.");
>+
>+  expect.ok(filename, "Filename is available.");
>+
>+  // Try to read back the saved dataURL
>+  var file = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsILocalFile);
>+  file.initWithPath(filename);
>+  expect.ok(file.exists(), "DataURL '" + file.path + "' has been saved.");

[13:24:31] whimboo: This should really be an assert. If it doesn't exist we would fail in loading it with a less helpful exception.
[13:25:49] whimboo: Hm, but is it really necessary to have this test? Wouldn't we already get a failure above when calling utils.saveScreenshot() and bailing out?
[14:59:39] xabolcs: Not sure about it. I vote for keeping it.
It's safe against black magic while we are in |waitFor|. :)
[15:54:59] whimboo: Depends on when ready is set. If it happens also for a failure case then another check would be necessary. But that happens with |expect.ok(!failure,| already, which should then be an assert too. If the file isn't existent it would be a hard-stop for us.






[1] - https://github.com/mozilla/mozmill/pull/94#discussion_r1700209

Comment 22

[Jeff Hammel]

I actually have no strong options about this.  I suppose I'd have to see both failure cases to really form one.  I'd be inclined to expect.ok, but as said, that's like a +0

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It doesn't make sense to continue the test if the save action didn't succeed. We will fail with a general less detailed exception then.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 665169 [details]
Patch v6 (patchset compareview at GitHub)

r=me with the change to use assert.ok to check the results of utils.saveScreenshot() but not later when already opening the file.

Also I thougth we wanted to rename this method to utils.saveToFile()?

Comment 25

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> Comment on attachment 665169 [details]
> Patch v6 (patchset compareview at GitHub)
> 
> r=me with the change to use assert.ok to check the results of
> utils.saveScreenshot() but not later when already opening the file.

Are you talking the expect.ok -> assert.ok change near L41?
This is already in the patch v6. [1]
It is commit 82d1826702f6 specifically. [2]

> 
> Also I thougth we wanted to rename this method to utils.saveToFile()?

I wanted to do the rename in another bug [3], not to conflict with Bug 762126.
Not yet filed.


[1] https://github.com/mozilla/mozmill/compare/997a3cf6e71d#L1R41
[2] https://github.com/mozilla/mozmill/commit/82d1826702f6
[3] https://github.com/mozilla/mozmill/pull/94#discussion_r1595891

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Szabolcs Hubai (:xabolcs) from comment #25)
> Are you talking the expect.ok -> assert.ok change near L41?
> This is already in the patch v6. [1]
> It is commit 82d1826702f6 specifically. [2]

This was the wrong place you have changed in that commit. I was talking about the expect.ok() calls which happen before those lines:

https://github.com/mozilla/mozmill/compare/997a3cf6e71d#L1R34

> > Also I thougth we wanted to rename this method to utils.saveToFile()?
> 
> I wanted to do the rename in another bug [3], not to conflict with Bug
> 762126.
> Not yet filed.

Ok, sounds fine then. Lets get it filed and marked as dependent bug. Thanks.

Comment 27

[Jeff Hammel]

Comment on attachment 665169 [details]
Patch v6 (patchset compareview at GitHub)

I don't have time to review this so I will go with henrik's judgment.

Comment 28

[Szabolcs Hubai (:xabolcs)]

Regarding the talk below, I 
- filed Bug 795243 and 
- added the checkin keyword.


(All times are +0200)

[ 9:57 ] Szabolcs: to clarify:
[ 9:57 ] Szabolcs: should I change the two expect.ok at L34 and L36?
[ 9:57 ] Szabolcs: and change back the assert.ok(file.exists(),...) to expect.ok() at L41?
[ 9:57 ] Szabolcs: https://github.com/mozilla/mozmill/compare/997a3cf6e71d#L1R34
[ 10:03 ] Henrik: when reading the code again i think 
[ 10:03 ] Henrik: we should make utils.saveScreenshot synchronous 
[ 10:03 ] Henrik: by moving the callback handing inside it. 
[ 10:03 ] Henrik: In that case your assert would be ok 
[ 10:03 ] Henrik: and we can remove the other two. 
[ 10:03 ] Henrik: i wouldn't mind to do it in a follow-up bug
[ 10:03 ] Henrik: this async stuff is adding a lot of complexity
[ 10:04 ] Szabolcs: uhh ... reading again :)
[ 10:07 ] Szabolcs: hmmm, that wouldn't be a hijack on the bug?
[ 10:07 ] Szabolcs: this bug was originally for creating binary files instead text files ...
[ 10:07 ] Henrik: as said lets do it separately
[ 10:08 ] Henrik: would you mind to file that bug?
[ 10:08 ] Henrik: in that case i think we can leave what you have right now
[ 10:08 ] Szabolcs: yes of course :)
[ 10:08 ] Henrik: and check it in
[ 10:08 ] Szabolcs: well, ok
[ 10:08 ] Szabolcs: thanks
[ 10:09 ] Szabolcs: should I note any dependecy for the new bug?
[ 10:10 ] Henrik: yes also give a comment on the bug
[ 10:10 ] Henrik: which states that we can checkin the current patch

Comment 29

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #26)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #25)
> > 
> > [...]
> > 
> > I wanted to do the rename in another bug [3], not to conflict with Bug
> > 762126.
> > Not yet filed.
> 
> Ok, sounds fine then. Lets get it filed and marked as dependent bug. Thanks.

Filed as Bug 795264.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Szabolcs Hubai (:xabolcs) from comment #29)
> > Ok, sounds fine then. Lets get it filed and marked as dependent bug. Thanks.
> 
> Filed as Bug 795264.

We also need a bug for the async code shuffled around. Have you filed it already? If yes, please mark it dependent.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Pushed to master:
https://github.com/mozilla/mozmill/commit/3fedc7a45148a89bae81dc8c684d3fe011cf1b2f

Comment 32

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #30)
> We also need a bug for the async code shuffled around. Have you filed it
> already? If yes, please mark it dependent.


(And in reply to Szabolcs Hubai (:xabolcs) from comment #28)
> Regarding the talk below, I 
> - filed Bug 795243 and 
> - added the checkin keyword.
> 

Added as dependent bug.