1308069 - Bing maps is incompletely updated.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot

[Tracking Requested - why for this release]: Rendering error


Steps To Reproduce:
1. Open https://www.bing.com/mapspreview
2. Repeating zoom in/out and pan

Actual Reuslts:
Bing maps is incompletely updated.
See attached screenshot

Expected Results:
Bing maps is updated properly.


Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=57647be72e76cb9c8b8ea6069c6d53362c9892d9&tochange=743d1b4c52dbf91559b049822c8caa02e86578d6

Triggered by: 743d1b4c52db	Edgar Chen — Bug 599975 - Fire error event for images with empty string src value; r=bz

Comment 1

[Marcia Knous [:marcia]]

Tracking 52+ for this regression.

Comment 2

[Mike Taylor [:miketaylr]]

Hm, I can't get this to reproduce on OSX or Windows 10. 

> 2. Repeating zoom in/out and pan

Anything more nuanced than just using the trackpad or zoom buttons to go in and out? Does this happen every time? Or on the Nth time?

Comment 3

[Alice0775 White]

(In reply to Mike Taylor [:miketaylr] from comment #2)
> Hm, I can't get this to reproduce on OSX or Windows 10. 
> 
> > 2. Repeating zoom in/out and pan
> 
> Anything more nuanced than just using the trackpad or zoom buttons to go in
> and out? Does this happen every time? Or on the Nth time?


Other reports see, http://forums.mozillazine.org/viewtopic.php?p=14715551#p14715551

I think it is not special operation, but slightly quick.
1. Turn mouse wheel in upward 4-5 tick and in downward 4-5 tick
2. Pan with mouse dragging
3. Repeat step 2-4

Often but not always.

Comment 4

[Alice0775 White]

Screen capture https://youtu.be/PbtTp2q3d1A

Comment 5

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Looks like it is caused by inconsistent behavior of image error event. Chrome/Edge seems clear pending error event if the src changed before it fired (Thanks the information provided by Mike in bug 1308126).

Test script: 
  http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4552

Result of Chrome and Edge:
  image2's error event is not fired.

And if I do the same thing in gecko (clearing pending error event), this issue isn't existed anymore.
But this behavior is not documented in the spec [1].

[1] https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data

Comment 6

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: WIP patch, v1

Comment 7

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Another test script:
  http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4554

Result of Chrome:
  image2's error event is only fired once.

Comment 8

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Filed a spec issue: https://github.com/whatwg/html/issues/1872

Comment 9

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Patch, v2

Comment 10

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> Created attachment 8799657 [details] [diff] [review]
> Patch, v2

It seems Chrome clears pending error event if src changed before it fired [1].
I wrote some test scripts (comment #5 and comment #7) and ran tests on Edge/Opera/Safari, they all have same result.
This behavior seems not be documented in the spec, but without it we will have this site-compat issue after landing bug 599975.

This patch do the similar things as other browsers.
And to minimize the changes, I mark error as suppressible only on src="" case, since this is the only case we have site-compat problem for now.

Hi Boris, do you have time to review this?
Thank you.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/ImageLoader.cpp?sq=package:chromium&rcl=1475738528&l=189

Comment 11

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7795f10764372e9c77955e489302bae5c1fcd6f8&filter-tier=1&group_state=expanded

Comment 12

[Boris Zbarsky [:bzbarsky]]

Edgar, thank you for digging into this.  I agree the spec here is moderately busted.

The right model in spec terms would be that the task queued in https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data step 9.2 should check for the algorithm having been reentered an no-op if so.  And probably similar for various other cases; I followed up in the spec issue.

I can review this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8799657 [details] [diff] [review]
Patch, v2

>+  // Suppress pending event if they are not fired. This is not documented in
>+  // the spec, but without this might cause site-compat problem (bug 1308069).

How about:

  // Pending load/error events need to be canceled in some situations.  This
  // is not documented in the spec, but can cause site compat problems if not
  // done.  See bug NNNNN and http://whatever

where NNNNN is the number of the bug you file to track the spec issue and http://whatever is the spec issue.  Once that's sorted out, we should update these comments and the code as needed.

Same for the other place where you have the comment.

>+nsImageLoadingContent::FireEvent(const nsAString& aEventType, bool aIsSuppressible)

I'd call the argument aIsCancelable.

>+  if (aIsSuppressible && aEventType.Equals(NS_LITERAL_STRING("error"))) {

Why the event type check?  aIsSuppressible should only be set to true in the cases we care about.

>+nsImageLoadingContent::ClearPendingEvent()

CancelPendingEvent?

>+  mozilla::WeakPtr<mozilla::LoadBlockingAsyncEventDispatcher> mPendingErrorEvent;

I'm a bit torn.  This is probably the simplest way to do this, but leaves the refcounted weakptr object (the one whose backref to the actual runnable gets nulled out when the runnable dies) hanging around for the lifetime of the image in general.

I just talked to smaug, and we settled on the following approach here:

1)  Add a virtual AsyncEventRunning(AsyncEventDispatcher*) function on EventTarget.  Have the default impl on EventTarget do nothing.

2)  In AsyncEventDispatcher::Run, right after the early return if canceled, call AsyncEventRunning() on mTarget.

3)  Override AsyncEventRunning() here to null out your pointer.

4)  File a followup bug to remove the existing ToggleEventDispatcher and FormPasswordEventDispatcher classes that could move to the new setup instead.

>+++ b/dom/base/test/test_bug1308069.html

The followup bug should make sure to note that this needs to become a web platform test.

>+    var imgTarge = new Image();

imgTarget?  Same throughout.  The comments have it right.

r=me with those nits.  Thank you again for hunting this down!

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> Comment on attachment 8799657 [details] [diff] [review]
> Patch, v2
> 
> >+  // Suppress pending event if they are not fired. This is not documented in
> >+  // the spec, but without this might cause site-compat problem (bug 1308069).
> 
> How about:
> 
>   // Pending load/error events need to be canceled in some situations.  This
>   // is not documented in the spec, but can cause site compat problems if not
>   // done.  See bug NNNNN and http://whatever
> 
> where NNNNN is the number of the bug you file to track the spec issue and
> http://whatever is the spec issue.  Once that's sorted out, we should update
> these comments and the code as needed.
> 
> Same for the other place where you have the comment.
> 

> >+++ b/dom/base/test/test_bug1308069.html
> 
> The followup bug should make sure to note that this needs to become a web
> platform test.
> 

Filed bug 1309461 to track spec issue.

Comment 15

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> Comment on attachment 8799657 [details] [diff] [review]
> Patch, v2
> 
> >+  mozilla::WeakPtr<mozilla::LoadBlockingAsyncEventDispatcher> mPendingErrorEvent;
> 
> I'm a bit torn.  This is probably the simplest way to do this, but leaves
> the refcounted weakptr object (the one whose backref to the actual runnable
> gets nulled out when the runnable dies) hanging around for the lifetime of
> the image in general.
> 
> I just talked to smaug, and we settled on the following approach here:
> 
> 1)  Add a virtual AsyncEventRunning(AsyncEventDispatcher*) function on
> EventTarget.  Have the default impl on EventTarget do nothing.
> 
> 2)  In AsyncEventDispatcher::Run, right after the early return if canceled,
> call AsyncEventRunning() on mTarget.
> 
> 3)  Override AsyncEventRunning() here to null out your pointer.
> 
> 4)  File a followup bug to remove the existing ToggleEventDispatcher and
> FormPasswordEventDispatcher classes that could move to the new setup instead.

Filed bug 1309508.

Comment 16

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Patch, v3, r=bz

Addressed review comment #13:
- Revise comments.
- s/aIsSuppressible/aIsCancelable/
- s/ClearPendingEvent/CancelPendingEvent/
- Use AsyncEventRunning() approach
- /imgTarge/imgTarget/

Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd688448bca32d3eda7351f6bdff7eba60bf83c9&filter-tier=1&group_state=expanded

Comment 17

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Patch, v4, r=bz

SVGFEImageElement will need to override AsyncEventRunning as well.

Try link for this version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3eeb2b6ec828a1fb582cb88596ed29820fc09ce1&filter-tier=1&group_state=expanded

Comment 18

[Carsten Book [:Tomcat]]

does not apply cleanly 

2 out of 3 hunks FAILED -- saving rejects to file dom/base/nsImageLoadingContent.cpp.rej
patching file dom/base/test/mochitest.ini
Hunk #1 FAILED at 624
1 out of 1 hunks FAILED -- saving rejects to file dom/base/test/mochitest.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_1308069_v4_r=bz.patch

Comment 19

[Boris Zbarsky [:bzbarsky]]

You need to implement AsyncEventRunning on the nsObjectLoadingContent subclasses too, right?

Comment 20

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> You need to implement AsyncEventRunning on the nsObjectLoadingContent
> subclasses too, right?

Ah, yes, we need to implement AsyncEventRunning on HTMLObjectElement and HTMLSharedObjectElement, too. Thanks for reminding.

Comment 21

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Patch, v5, r=bz

Implement AsyncEventRunning on HTMLObjectElement and HTMLSharedObjectElement.

Try link for this version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b425ab2eebcf86f96228ee62350dacbd54de0a3&group_state=expanded&filter-tier=1

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/688b4667b810
Clear pending error event fired by src="" case if src changed before it fired. r=bz

Comment 23

[Gerry Chang [:gchang]]

Track 51+ as regression in bing map.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/688b4667b810

Comment 25

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: [aurora] Patch, v5, r=bz

Patch for aurora

Comment 26

[Gerry Chang [:gchang]]

Hi :edgar,
Can you create an uplift request for 51 aurora?

Comment 27

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8801986 [details] [diff] [review]
[aurora] Patch, v5, r=bz

Approval Request Comment
[Feature/regressing bug #]:
A regression of bug 599975.

[User impact if declined]:
We will have inconsistent behavior on error event with other browsers (Chrome/Edge/Safari) which lead to Bing maps incompletely being updated.

[Describe test coverage new/current, TreeHerder]:
New test added and image's error event is also covered by other existing tests.

[Risks and why]:
Fairly low. This patch only affect the error event triggered by src="" case.

[String/UUID change made/needed]:
None.

Comment 28

[Gerry Chang [:gchang]]

Comment on attachment 8801986 [details] [diff] [review]
[aurora] Patch, v5, r=bz

Fix a regression related to Bing map. Take it in 51 aurora.

Comment 29

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c969b37d0973

Comment 30

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c969b37d0973

Comment 31

[Timea Cernea [:tbabos][inactive]]

Hi,

I can reproduce this issue on the latest Nigthly (only x32 version) on Windows 7. Here is a screen recording and screenshot: https://streamable.com/hzu5k and https://imgur.com/HOKx7xc.

Should I log a new bug or will it be investigated in this one?

Thanks!

Comment 32

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Please file a new bug. Thank you.

Comment 34

[Timea Cernea [:tbabos][inactive]]

Thanks Edgar!
Submitted Bug 1503603 to handle the case mentioned in Comment 31.