Closed Bug 1145903 Opened 9 years ago Closed 8 years ago

Intermittent bug917595-exif-rotated.jpg | image comparison (==), max difference: 224, number of differing pixels: 434985

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Version:
39 Branch
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1251088
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- affected

People

(Reporter: KWierso, Assigned: BenWa)

References

Details

(Keywords: intermittent-failure, Whiteboard: [rr-chaos])

Attachments

(1 file)

MozReview Request: Bug 1145903 - Use CSS to shrinkToFit instead of a racy set of width/height on the image frame. r=seth
58 bytes, text/x-review-board-request
tnikkel
: review+
Details 
16:22:31 INFO - REFTEST INFO | [CONTENT] MozInvalidateEvent didn't invalidate
16:22:31 INFO - REFTEST TEST-LOAD | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-1-ref.html | 11653 / 11944 (97%)
16:22:31 INFO - REFTEST INFO | [CONTENT] MozInvalidateEvent didn't invalidate
16:22:31 INFO - REFTEST fuzzy match
16:22:31 INFO - REFTEST TEST-PASS | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-iframe-1.html | image comparison (==)
16:22:31 INFO - REFTEST INFO | Loading a blank page
16:22:31 INFO - REFTEST TEST-END | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-iframe-1.html
16:22:31 INFO - REFTEST TEST-START | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-exif-rotated.jpg
16:22:31 INFO - REFTEST TEST-LOAD | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-exif-rotated.jpg | 11654 / 11944 (97%)
16:22:31 INFO - REFTEST TEST-LOAD | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-pixel-rotated.jpg | 11654 / 11944 (97%)
16:22:32 INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-exif-rotated.jpg | image comparison (==), max difference: 224, number of differing pixels: 434985

http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-macosx64/1426886975/fx-team_snowleopard_test-reftest-bm106-tests1-macosx-build179.txt.gz&only_show_unexpected=1

16:22:32 INFO - REFTEST INFO | Loading a blank page
16:22:32 INFO - REFTEST TEST-END | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug917595-exif-rotated.jpg
16:22:32 INFO - REFTEST TEST-START | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug1106522-1.html
16:22:32 INFO - REFTEST TEST-LOAD | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug1106522-1.html | 11655 / 11944 (97%)
16:22:32 INFO - REFTEST TEST-LOAD | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug1106522-ref.html | 11655 / 11944 (97%)
16:22:32 INFO - REFTEST TEST-PASS | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug1106522-1.html | image comparison (==)
16:22:32 INFO - REFTEST INFO | Loading a blank page
16:22:32 INFO - REFTEST TEST-END | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug1106522-1.html
16:22:32 INFO - REFTEST TEST-START | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/dom/html/reftests/bug1106522-2.html
Inactive; closing (see bug 1180138).
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
I was able to reproduce this using rr chaos using only 17 runs.
Whiteboard: [rr-chaos]
I wonder if it's a similar issue to bug 1226748.
Thanks to RR for this bug!

Basically what's happening here is we set the size of the image frame when we get the 'load' event on the image document. However we haven't loaded the CSS style sheet yet which we need to get the 'image-orientation' style. During the race condition we don't have 'image-orientation' so we ignore the exif rotation from the image.

By using CSS to shrinkToFit we don't need to size the image frame and thus CSS updates the size properly when the CSS style is loaded.
Assignee: nobody → bgirard
(In reply to Benoit Girard (:BenWa) from comment #42)
> Basically what's happening here is we set the size of the image frame when
> we get the 'load' event on the image document. However we haven't loaded the
> CSS style sheet yet

If the document has had the load event, then shouldn't any style sheets that it is using also be loaded, parsed, and applied?
(In reply to Timothy Nikkel (:tnikkel) from comment #43)
> (In reply to Benoit Girard (:BenWa) from comment #42)
> > Basically what's happening here is we set the size of the image frame when
> > we get the 'load' event on the image document. However we haven't loaded the
> > CSS style sheet yet
> 
> If the document has had the load event, then shouldn't any style sheets that
> it is using also be loaded, parsed, and applied?

Good question. I haven't had time to look into that part. There's really two problems here. There's the data being wrong at the load events, and there's the data being static after it changes. My patch fixes the second part which is the most important. But it would be nice to fix the 'load' event as well. It needs more investigation.
Comment on attachment 8722234 [details]
MozReview Request: Bug 1145903 - Use CSS to shrinkToFit instead of a racy set of width/height on the image frame. r=seth

:tn do you mind reviewing this?
Attachment #8722234 - Flags: review?(seth) → review?(tnikkel)
Comment on attachment 8722234 [details]
MozReview Request: Bug 1145903 - Use CSS to shrinkToFit instead of a racy set of width/height on the image frame. r=seth

https://reviewboard.mozilla.org/r/35927/#review32963
Attachment #8722234 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #43)
> (In reply to Benoit Girard (:BenWa) from comment #42)
> > Basically what's happening here is we set the size of the image frame when
> > we get the 'load' event on the image document. However we haven't loaded the
> > CSS style sheet yet
> 
> If the document has had the load event, then shouldn't any style sheets that
> it is using also be loaded, parsed, and applied?

This sounds like a serious bug if true. We should investigate this while you have a trace.
(In reply to Timothy Nikkel (:tnikkel) from comment #43)
> (In reply to Benoit Girard (:BenWa) from comment #42)
> > Basically what's happening here is we set the size of the image frame when
> > we get the 'load' event on the image document. However we haven't loaded the
> > CSS style sheet yet
> 
> If the document has had the load event, then shouldn't any style sheets that
> it is using also be loaded, parsed, and applied?

If it was the load event for the image, and not the image _document_, that might make more sense.
The listener for that appears to be added here:

http://mxr.mozilla.org/mozilla-central/source/dom/html/ImageDocument.cpp#261

so it's listening for the load event of the img element. So then it makes sense.
So you're saying this matches with what I saw in the replay? Do we still need to file a new bug to improve the semantics of the 'load' event for ImageDocument?

I wonder how much work it would be to remove the 'little' ammount that happens on that 'load' event. I'm not really the best person to do it honestly. But right now the image document is trying to keep the image size up to date but it's doing a terrible job at it.
(In reply to Benoit Girard (:BenWa) from comment #51)
> So you're saying this matches with what I saw in the replay? Do we still
> need to file a new bug to improve the semantics of the 'load' event for
> ImageDocument?

Line 596 isn't the load event for the image document, it's the load event for the img element that is inside the image document (the image document being a small html document). Since it is only the load event of the img element we don't expect a style sheet associated with the document to be loaded yet. So what you observed in replay makes sense.

What gets stored in mImageWidth/Height still depends on the ordering of the img element load event and when the CSS property image-orientation gets applied, correct? So that would still be a bug.

Seems like we could easily solve that by calling UpdateSizeFromLayout() in ImageDocument::OnLoadComplete (which is the image document load notification). mImageWidth/Height would be wrong before load, but at least we correct it.
Yes, but it will still be wrong if something causes the image width/height to change. Which AFAIK is pretty much just the inspector changes.
(In reply to Benoit Girard (:BenWa) from comment #53)
> Yes, but it will still be wrong if something causes the image width/height
> to change. Which AFAIK is pretty much just the inspector changes.

Talked to Benoit on irc. We agreed using the devtools to edit the image doc isn't a very important case to worry about.
(In reply to Timothy Nikkel (:tnikkel) from comment #52)
> Seems like we could easily solve that by calling UpdateSizeFromLayout() in
> ImageDocument::OnLoadComplete (which is the image document load
> notification). mImageWidth/Height would be wrong before load, but at least
> we correct it.

OnLoadComplete is for the load notification for the image coming from imagelib. So it is also not the image document load.
(In reply to Timothy Nikkel (:tnikkel) from comment #55)
> (In reply to Timothy Nikkel (:tnikkel) from comment #52)
> > Seems like we could easily solve that by calling UpdateSizeFromLayout() in
> > ImageDocument::OnLoadComplete (which is the image document load
> > notification). mImageWidth/Height would be wrong before load, but at least
> > we correct it.
> 
> OnLoadComplete is for the load notification for the image coming from
> imagelib. So it is also not the image document load.

Filed bug 1251088 for this.
Depends on: 1251796
Backed out because bug 1251796 (which this caused) is more severe than this one
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c4f2fb86e9
I'm waiting on bug 1251280 to be fixed so that I don't modify stuff on top of broken behavior.

After that I'll look if removing width/height 100% prevents bug 1251796.
Depends on: 1251280
Bug 1251088, which I just landed will probably fix this. It fixes the problem at the document load event. The image will still appear with the wrong aspect ratio before that point (there's a bug for that but I can't find it right now), so something like the patch in this bug is still wanted.
Thanks Timothy!
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
Resolution: --- → DUPLICATE
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: