Closed Bug 1180991 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Page Info Window, defect)

42 Branch
x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
e10s m8+ ---
firefox44 --- fixed

People

(Reporter: magicp.jp, Assigned: mconley)

References

Details

(Whiteboard: [bugday-20151104])

Attachments

(5 files)

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.
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
Component: Untriaged → Page Info Window
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Ever confirmed: true
OS: Windows 8.1 → All
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.
Blocks: 996785
Assignee: nobody → mconley
Hey jimicy - is your patch going to fix this?
Flags: needinfo?(jimmyw22)
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.
Flags: needinfo?(jimmyw22) → needinfo?(magicp.jp)
(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.
Please find attached file "Bug 1180991 - Step to reproduce"
Flags: needinfo?(magicp.jp)
Assignee: mconley → jimmyw22
(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.
Jimmy, will you keep working on these?
Flags: needinfo?(jimmyw22)
Hi Brad, yes I would like to continue to work on this.
Flags: needinfo?(jimmyw22)
Summary: [e10s] Page Info: Media preview is not shown on multi-process Nightly → [e10s] Page Info: Media preview is not shown the first time on multi-process Nightly
Assignee: jimmyw22 → mconley
Bug 1180991 - Send up natural dimensions of images loaded in content for Page Info. r?florian
Attachment #8669861 - Flags: review?(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?
Attachment #8669861 - Flags: review?(florian)
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.
Needinfo'ing for the edge case question in comment 13.
Flags: needinfo?(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.
Flags: needinfo?(florian)
(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?
Flags: needinfo?(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.
Flags: needinfo?(florian)
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 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
Attachment #8669861 - Attachment description: MozReview Request: Bug 1180991 - Send up natural dimensions of images loaded in content for Page Info. r?florian → MozReview Request: Bug 1180991 - Send up natural dimensions of background images loaded in content for Page Info. r?florian
Attachment #8669861 - Flags: review?(florian)
Attachment #8669861 - Flags: review?(florian) → review+
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
Thanks, florian!
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
https://hg.mozilla.org/mozilla-central/rev/a12d60c18af9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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
QA Whiteboard: [bugday-20151014]
Still reproduce in the latest Nightly 44.0a1
Please find attached movie
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Are you able to reproduce on non about: pages?

If not, I suspect this is a distinct bug that we should file separately.
Flags: needinfo?(magicp.jp)
Yes, reproduce at https://addons.mozilla.org
Please find attached movie
Flags: needinfo?(magicp.jp)
Okay, confirmed - I'm seeing that too. I'll investigate this tomorrow.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Filed bug 1215589 for the background image bug.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20151104]
You need to log in before you can comment on or make changes to this bug.