1192536 - NodeActor.getImageData() should wait for image to finish loading before generating the preview

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Sami Jaktholm]

I was poking around the inspector code and noticed that NodeActor.getImageData() does not ensure the image is loaded before the preview is generated. If the image happens to be loading when the data is requested, it could lead to intermittent bugs that are hard to reproduce and debug.

IMO it'd make sense to wait for the load to complete before the preview is generated. However, there should be a timeout similar to InspectorActor.getImageDataFromURL() method after which the preview generation aborts with a clear reason to avoid blocking the actor for too long periods.

Comment 1

[Sami Jaktholm]

Attachment: bug-1192536-wait-for-images-to-load-1.patch

The Inspector related actors have two methods for extracting image data: InspectorActor.getImageDataFromURL() and NodeActor.getImageData(). Since they both use imageToImageData() method to generate the actual data-uri and are only users of that method this patch modifies imageToImageData to wait for the image to load before generating the data-uri.

It extracts the load-detection logic from getImageDataFromURL(), cleans it up a bit and makes it work with images that have already finished loading.

Comment 2

[Sami Jaktholm]

Attachment: bug-1192536-wait-for-images-to-load-2-tests.patch

Here's a few server side tests for both getImageData() and getImageDataFromURL().

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=171765d04a20

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8648337 [details] [diff] [review]
bug-1192536-wait-for-images-to-load-1.patch

Review of attachment 8648337 [details] [diff] [review]:
-----------------------------------------------------------------

r+, no remarks at all, the code is simpler, and easier to read. Thanks.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8648338 [details] [diff] [review]
bug-1192536-wait-for-images-to-load-2-tests.patch

Review of attachment 8648338 [details] [diff] [review]:
-----------------------------------------------------------------

Why choose browser mochitest over chrome mochitest here? We already have a bunch of inspector-related server tests in toolkit/devtools/server/tests/mochitest which are chrome mochitests. I like chrome test because they run in the content process (with chrome privileges) so it's easy to access content objects when needed. But I don't like them because they are in HTML pages, which makes them more verbose, harder to read.
So I don't see any big reason to choose one over the other (unless maybe one is faster to run), but I would tend to keep all inspector-server tests in one test dir, rather than start creating new tests in a different directory.

Apart from this, the tests look ok to me. So let's just have a discussion about where they should go and I'll R+.

Comment 5

[Sami Jaktholm]

Oh, I didn't even remember that those existed. Anyway, it really does make sense to keep all the tests in the same place. I'll modify the tests!

Comment 6

[Sami Jaktholm]

Attachment: bug-1192536-wait-for-images-to-load-1.patch

A new patch with updated commit message.

Comment 7

[Sami Jaktholm]

Attachment: bug-1192536-wait-for-images-to-load-2-tests.patch

Here are the tests as chrome mochitests.

Pending try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32568bd37aca

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/57b63992d0c2
https://hg.mozilla.org/integration/fx-team/rev/b6def4e57d9c

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57b63992d0c2
https://hg.mozilla.org/mozilla-central/rev/b6def4e57d9c