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

RESOLVED DUPLICATE of bug 1251088

Status

()

defect
RESOLVED DUPLICATE of bug 1251088
4 years ago
10 months ago

People

(Reporter: KWierso, Assigned: BenWa)

Tracking

({intermittent-failure})

39 Branch
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 affected)

Details

(Whiteboard: [rr-chaos])

Attachments

(1 attachment)

Reporter

Description

4 years ago
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
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Inactive; closing (see bug 1180138).
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 39

3 years ago
I was able to reproduce this using rr chaos using only 17 runs.
Whiteboard: [rr-chaos]
Assignee

Comment 40

3 years ago
I wonder if it's a similar issue to bug 1226748.
Assignee

Comment 42

3 years ago
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?
Assignee

Comment 44

3 years ago
(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.
Assignee

Comment 45

3 years ago
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.
Assignee

Comment 51

3 years ago
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.
Assignee

Comment 53

3 years ago
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.
Backed out because bug 1251796 (which this caused) is more severe than this one
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c4f2fb86e9
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 60

3 years ago
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
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
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.
Assignee

Comment 66

3 years ago
Thanks Timothy!
Assignee

Updated

3 years ago
Duplicate of this bug: 1242089
Assignee

Updated

3 years ago
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1251088

Updated

10 months ago
Product: Core → Core Graveyard

Updated

10 months ago
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.