1180991 - [e10s] Page Info: Media preview is not shown the first time on multi-process Nightly

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[magicp]

User Agent: Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150706030206

Steps to reproduce:

1) Run Nightly 42.0a1 with enable multi-process
2) Open Nightly Start Page
3) Right click and select "View Page Info"
4) Select "Media" Pane
5) Select one by one a image
6) Media preview is not shown a image in the first time click


Actual results:

Media preview is not shown a image in the first time click. However, after move the other image, the second click works well.

Disable multi-process: It works fine!


Expected results:

Media preview should be shown a image in the e10s window.

Comment 1

[magicp]

Regression Bug 1158574 ?
https://bugzilla.mozilla.org/show_bug.cgi?id=1158574

Comment 2

[magicp]

Confirmed on...
- Windows 8.1 32bit
- Windows 10 Pro Insider Preview 64bit (Build 10162)
- Ubuntu 14.04 64bit
- Mac OS X Yosemite 10.10.4 64bit

Comment 3

[Alice0775 White]

I think this is e10s implementation level bug. And reproduced since landing Bug 996785.

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=885e99ddd324&tochange=4f0a7452db01

Before landing Bug 996785 : No media pane appears
After landing Bug 996785 : Media pane appears, but Media preview does not appear in the first click of the list items.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Hey jimicy - is your patch going to fix this?

Comment 5

[Jimmy Wang (:jimicy) - works on e10s stuff]

magicp could you provide the website that you used to test this?
Better yet, would you be able to provide a video recording?

Using this page as an example
Open a private browsing window (calling page info on a page again has a different behavior on image preview. They all display)

Then
Select one by one a image
By using the down arrow to go through them
But I do get a media preview for the images on their first preview.

Only the first image doesn't make a first preview when you switch to the media tab.
First image doesn't make a first preview in release firefox either when you first open page info and select media tab. The image preview displays after switching to another image and then back which you did state.

Comment 6

[Alice0775 White]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #5)
> magicp could you provide the website that you used to test this?
> Better yet, would you be able to provide a video recording?

any page. e.g https://wiki.mozilla.org/Main_Page 

> 
> Using this page as an example
> Open a private browsing window (calling page info on a page again has a
> different behavior on image preview. They all display)

I can reproduce the problem regardless of private browsing mode

> 
> Then
> Select one by one a image
> By using the down arrow to go through them
> But I do get a media preview for the images on their first preview.
> 

No image is shown at first select in most cases

> Only the first image doesn't make a first preview when you switch to the
> media tab.
> First image doesn't make a first preview in release firefox either when you
> first open page info and select media tab. The image preview displays after
> switching to another image and then back which you did state.

This should be another bug.

Comment 7

[magicp]

Attachment: Bug 1180991 - Step to reproduce.mp4

Please find attached file "Bug 1180991 - Step to reproduce"

Comment 8

[Jimmy Wang (:jimicy) - works on e10s stuff]

(In reply to magicp from comment #7)
> Created attachment 8630866 [details]
> Bug 1180991 - Step to reproduce.mp4
> 
> Please find attached file "Bug 1180991 - Step to reproduce"

Thanks magicp! I see what you mean now.
I can also reproduce it. Image elements with the type background on the media tab don't have an image preview when you click on them the first time and have improperly sizes 0px x 0px the first time as well.

Whereas image elements of type image do have a image preview when you click on them the first time.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Jimmy, will you keep working on these?

Comment 10

[Jimmy Wang (:jimicy) - works on e10s stuff]

Hi Brad, yes I would like to continue to work on this.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian

Bug 1180991 - Send up natural dimensions of images loaded in content for Page Info. r?florian

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8669861 [details]
MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian

https://reviewboard.mozilla.org/r/21273/#review19225

Random fact: with this patch you are editing the code I added in my very first patch in bug 229441.

::: browser/base/content/content.js:1162
(Diff revision 1)
> +      let img = content.document.createElement("img");

What's the reason for creating a new element in the content document? Wouldn't item.natural\* give the same results?

Or do we need to do this only for background images?

::: browser/base/content/pageinfo/pageInfo.js:907
(Diff revision 1)
> -        newImage.width = ("width" in item && item.width) || newImage.naturalWidth;
> +        newImage.width = ("width" in item && item.width) || item.naturalWidth || newImage.naturalWidth;

Do we still have an edge case where we'll be setting the width and height to 0?

::: browser/base/content/pageinfo/pageInfo.js:913
(Diff revision 1)
> -        newImage.width = newImage.naturalWidth;
> +        newImage.width = item.naturalWidth || newImage.naturalWidth;

Do we expect to receive the natural\* values from the content process for background images?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/21273/#review19225

> What's the reason for creating a new element in the content document? Wouldn't item.natural\* give the same results?
> 
> Or do we need to do this only for background images?

Yes - we want the natural dimensions for both images and background images, but background images need to be specially loaded like this so that we can get those dimensions.

> Do we still have an edge case where we'll be setting the width and height to 0?

I'm not sure - which edge case are you referring to?

> Do we expect to receive the natural\* values from the content process for background images?

With my patch, we do, yes.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Needinfo'ing for the edge case question in comment 13.

Comment 15

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)

> > Do we still have an edge case where we'll be setting the width and height to 0?
> 
> I'm not sure - which edge case are you referring to?

Well, I guess if we have the image loaded neither in the content nor in the chrome process, we'll be setting the dimensions to 0 and the only way to fix that would be to change the dimensions from a load observer on the image tag in the page info dialog... but I guess that edge case existed before and isn't even really relevant, because Page Info is only supposed to be listing images that are shown in the page.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> 
> > > Do we still have an edge case where we'll be setting the width and height to 0?
> > 
> > I'm not sure - which edge case are you referring to?
> 
> Well, I guess if we have the image loaded neither in the content nor in the
> chrome process, we'll be setting the dimensions to 0 and the only way to fix
> that would be to change the dimensions from a load observer on the image tag
> in the page info dialog... but I guess that edge case existed before and
> isn't even really relevant, because Page Info is only supposed to be listing
> images that are shown in the page.

That's correct, this bug exists already without e10s and without my patch.

I can certainly file that, but do you think it prevents us from landing this?

Comment 17

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #16)
> do you think it prevents us from landing this?

No.

The reason for not r+'ing the current patch was that I expect the createElement("img") thing to either be done only for background images (preferred), or a comment to be added to explain why we are doing it (ie. because the information is known for some images, but not all at that point). Actually, unless I'm confused, the |item instanceof Ci.nsIImageLoadingContent| test in the current patch seems to avoid creating the img element for background images and only do it for existing <img> tags.

I'm not even sure the edge case discussed above is worth filing a bug (we won't get to fixing it, right?). Maybe just rephrase a little bit the end of the comment, ie. "we'll assume that we can use the natural dimensions of the newImage we just created." should imply that when the natural dimensions of the newImage aren't known, then the image is mostly likely broken so dimensions don't matter.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/21273/#review19225

> I'm not sure - which edge case are you referring to?

Alright, dropping this as per https://bugzilla.mozilla.org/show_bug.cgi?id=1180991#c17

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669861 [details]
MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian

Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8669861 [details]
MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian

https://reviewboard.mozilla.org/r/21273/#review19693

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Thanks, florian!

Comment 22

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/a12d60c18af96fbd0a5064b8a1daf2adb3476b20
Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r=florian

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/a12d60c18af9

Comment 24

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced this bug on Nightly 42.0a1 (2015-07-06) (Build ID: 20150706030206) by following Comment 0's instruction on Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Nightly 44.0a1 (2015-10-15)

Build ID: 20151015030233
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0

Comment 25

[magicp]

Attachment: Bug 1180991 - still reproduce.mp4

Still reproduce in the latest Nightly 44.0a1
Please find attached movie

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Are you able to reproduce on non about: pages?

If not, I suspect this is a distinct bug that we should file separately.

Comment 27

[magicp]

Attachment: Bug 1180991 - [e-mail]4

Yes, reproduce at https://addons.mozilla.org
Please find attached movie

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Okay, confirmed - I'm seeing that too. I'll investigate this tomorrow.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

It looks like when background images are in block that's not been displayed that the images need to be loaded, and that occurs asynchronously. By the time the naturalWidth / naturalHeight is computed, the serialization of the node has already occurred.

So the problem that magicp is running into is a special-case of this bug.

I'm going to mark this bug RESOLVED again, and open a new bug for this special-case which can be fixed separately.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Filed bug 1215589 for the background image bug.

Comment 31

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 42.0a1 (Build ID: 20150706030206) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 45.0a1 (Build ID: 20151105030433)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

As it is also verified on Linux (Comment 24), I am marking this bug verified.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1180991.html