Closed Bug 1308069 Opened 3 years ago Closed 3 years ago

Bing maps is incompletely updated.

Categories

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

51 Branch
x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + fixed
firefox52 + fixed

People

(Reporter: alice0775, Assigned: edgar)

References

Details

(Keywords: regression, site-compat, Whiteboard: [parity-chrome][parity-edge])

Attachments

(3 files, 4 obsolete files)

Attached image 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
Assignee: nobody → echen
Tracking 52+ for this regression.
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?
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
Screen capture https://youtu.be/PbtTp2q3d1A
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
Attached patch WIP patch, v1 (obsolete) — Splinter Review
Priority: -- → P1
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.
Attached patch Patch, v2 (obsolete) — Splinter Review
Attachment #8798799 - Attachment is obsolete: true
See Also: → 1308126
(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
Flags: needinfo?(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 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!
Flags: needinfo?(bzbarsky)
Attachment #8799657 - Flags: review+
See Also: → 1309461
(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.
(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.
Attached patch Patch, v3, r=bz (obsolete) — Splinter Review
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
Attachment #8799657 - Attachment is obsolete: true
Attachment #8800214 - Flags: review+
Attached patch Patch, v4, r=bz (obsolete) — Splinter Review
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
Attachment #8800214 - Attachment is obsolete: true
Attachment #8800551 - Flags: review+
Keywords: checkin-needed
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
Flags: needinfo?(echen)
Keywords: checkin-needed
You need to implement AsyncEventRunning on the nsObjectLoadingContent subclasses too, right?
(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.
Attached patch Patch, v5, r=bzSplinter Review
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
Attachment #8800551 - Attachment is obsolete: true
Flags: needinfo?(echen)
Attachment #8800999 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Track 51+ as regression in bing map.
https://hg.mozilla.org/mozilla-central/rev/688b4667b810
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Patch for aurora
Hi :edgar,
Can you create an uplift request for 51 aurora?
Flags: needinfo?(echen)
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.
Attachment #8801986 - Flags: approval-mozilla-aurora?
Flags: needinfo?(echen)
Comment on attachment 8801986 [details] [diff] [review]
[aurora] Patch, v5, r=bz

Fix a regression related to Bing map. Take it in 51 aurora.
Attachment #8801986 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(echen)
Please file a new bug. Thank you.
Flags: needinfo?(echen)
Duplicate of this bug: 1308126
Thanks Edgar!
Submitted Bug 1503603 to handle the case mentioned in Comment 31.
You need to log in before you can comment on or make changes to this bug.