1567535 - Google Maps fail to render correctly on Firefox 68.0.1 after reload

Status: VERIFIED DUPLICATE of bug 1565542

(Core :: Graphics: ImageLib, defect)

Description

[Steve Robinson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Visit this bare-bones Google Maps example page (or any Google Map) on a Mac in Firefox 68 - 68.0.1 : https://developers.google.com/maps/documentation/javascript/examples/map-simple
After the page loads click in the address bar - hit return to re-load the page (not the refresh button).
The bug also occurs on first load if the map loads out of the viewport.

Actual results:

The Map either fails to render completely or only partially renders leaving grey patches where there should be map imagery.

Expected results:

Should have rendered completely.
This only started after version 68.

Comment 1

[Alice0775 White]

I can reproduce the issue on Firefox68.0.1 Windows10.

Comment 2

[Alice0775 White]

Tested on Windows10 without WebRender.
If maximized(19361066), Completely Gray rectangle, no maps no icons are display when reloading.
If not maximized(1290982), Zoom icons, Fullscreen icon, Street View icon, Map button, Satellite button are sometime missing when reloading.

Reproducible: 100%(1st reload, 3rd reload, 5th reload)

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d9a454afe9bf4e6299021d96948f135ecbc827ca&tochange=c477a96a488995459baab0948a10cb7231d7cbe3

Regressed by: c477a96a488995459baab0948a10cb7231d7cbe3
Ehsan Akhgari — Bug 1548349 - Make sure the image cache for third-party tracking subresources is keyed to the top-level document's eTLD+1; r=baku,aosmond

:Ehsan Akhgari,
Your patch seems to cause the regression. Can you please look into this?

Comment 3

[(no longer active)]

Sure, I'll look on Monday. Thanks for bisecting!

Comment 6

[(no longer active)]

What happens here is that we load the maps images from the image cache after the second load, and their ImageCacheKey::mTopLevelBaseDomain is set to "google.com" (since the images are loaded inside an iframe). But the second time that the images are loaded they aren't painted for some reason...

Comment 7

[Kohei Yoshino [:kohei]]

[Tracking Requested - why for this release]: Affecting popular Google Maps; 2 duplicates.

Comment 8

[Mike Taylor [:miketaylr]]

Does this affect everyone? Or only people with ETP and new profiles?

(I can repro on Mac in 69, and AFAIK I don't have ETP enabled)

Comment 9

[(no longer active)]

(In reply to Mike Taylor [:miketaylr] from comment #8)

Does this affect everyone? Or only people with ETP and new profiles?

(I can repro on Mac in 69, and AFAIK I don't have ETP enabled)

Unfortunately it impacts everyone. The bug itself has nothing to do with ETP, but with the change in the image cache key change in bug 1548349.

FWIW, I tried to create a simple map app based on the instructions on this page (https://sideways-jumper.glitch.me/) and the bug reproduces there as well...

Comment 10

[(no longer active)]

Attachment: imgRequest log

This is the log captured by loading https://sideways-jumper.glitch.me/ in a new profile and reloading the page with MOZ_LOG=imgRequest:5 set.

Comment 11

[(no longer active)]

Perhaps I'm completely wrong here, but it seems like the bug has something to do with the images being discarded prematurely... Here is what I have been able to discover so far:

• From the attached log, if you follow along what happens to this image (as an example) before an after reload, you'll notice that the SIZE_AVAILABLE, FRAME_UPDATE, FRAME_COMPLETE and DECODE_COMPLETE notifications are dispatched in the same order in both cases. However, if you look at the notifications this imgRequestProxy at 0x7f13fb082c80 receives, you'll see that between the two reloads, it receives a DISCARD notification.
• Setting the image.mem.discardable pref to false makes the bug go away.

Could it be that we need to somehow ensure that the images in the image cache do not get discarded prematurely?

Comment 12

[(no longer active)]

(BTW I have this bug recorded in the following Pernosco session for easier debugging: https://pernos.co/debug/YFaxs3kTGu9fctZXxNoneg/index.html)

Comment 13

[(no longer active)]

Here is another interesting piece of discovery. For the interesting images (the ones that do not show up after reload), the first time the page loads, at https://searchfox.org/mozilla-central/rev/1dfd70469212ef2785d41827c5532c571c784227/dom/base/nsImageLoadingContent.cpp#1655 the image is inside the composed doc, so GetOurCurrentDoc() returns a valid document, but the second time the image isn't yet injected into the document, so GetOurCurrentDoc() returns nullptr and we bail out early.

Comment 14

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #13)

Here is another interesting piece of discovery. For the interesting images (the ones that do not show up after reload), the first time the page loads, at https://searchfox.org/mozilla-central/rev/1dfd70469212ef2785d41827c5532c571c784227/dom/base/nsImageLoadingContent.cpp#1655 the image is inside the composed doc, so GetOurCurrentDoc() returns a valid document, but the second time the image isn't yet injected into the document, so GetOurCurrentDoc() returns nullptr and we bail out early.

More on this. When the bug doesn't happen, there are 36 images that get loaded inside the https://numerous-switch.glitch.me/ iframe (embedded on the https://sideways-jumper.glitch.me/ test page). When the bug does happen, there are no img elements on that page. This can be verified in devtools by focusing on that iframe and running document.querySelectorAll("img") in the web console. So what I discovered in comment 13 could just be explained by the fact that in the buggy scenario, something is preventing the page from injecting the img elements into the document...

Comment 15

[(no longer active)]

Attachment: Page using image.decode() promise

The page has code like this. The first breakpoint is hit, but the second one is never hit. Timothy Nikkel suggested that we may be looking at bug 1565542 here.

Comment 16

[(no longer active)]

As I was getting desparate, I started to back out individual parts of https://hg.mozilla.org/mozilla-central/rev/c477a96a4889 to see if the regression goes away. It turns out that reverting the changes made to ImageCacheKey::GetSpecialCaseDocumentToken() is sufficient to fix the regression. I'm not sure I understand why though (or what was actually causing the regression in the first place!)

Comment 17

[(no longer active)]

Attachment: Bug 1567535 - Restore the changes to ImageCacheKey::GetSpecialCaseDocumentToken() to satisfy the image cache gods;

Comment 18

[(no longer active)]

Speaking to Timothy on IRC, we are suspecting that this may be a timing issue and the way this patch is helping could be by adding some extra code to run in the right places. He's looking into bug 1565542 today to see if that's an easy fix, since that may have something to do with the issue here. If that doesn't go anywhere we decided to go ahead with this approach and hope that if this was just a timing fix we can discover that in QA testing.

STR for QA testing:

1. In a new profile, load https://sideways-jumper.glitch.me/ in a new tab.
2. See if the map is visible.
3. Reload.
4. Verify that the map is still visible.

You may repeat the above steps many times, since this may be a timing dependent issue.

Comment 19

[(no longer active)]

I think I now realize why this patch has an impact... These images are inside a third-party document, so AntiTrackingCommon::MaybeIsFirstPartyStorageAccessGrantedFor() with the image URLs can return false, and before my patch mControlledDocument for the entries would turn out nullptr, whereas after it would turn out to be a unique pointer, so they would be considered separate entries inside the image cache.

That means that my patch may just be making this code not hit bug 1565542 after all...

Comment 20

[Timothy Nikkel (:tnikkel)]

I posted a patch to bug 1565542 so hopefully we don't need this.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Do we have any thoughts on the relative risk of doing the partial backout from this bug for 68.0.2 vs. forward-fixing via bug 1565542? Note that I'm only worried about the dot release as far as that question is concerned.

Comment 22

[Timothy Nikkel (:tnikkel)]

The patch in bug 1565542 doesn't feel that risky to me.

Comment 23

[(no longer active)]

(In reply to Timothy Nikkel (:tnikkel) from comment #20)

I posted a patch to bug 1565542 so hopefully we don't need this.

That description makes perfect sense based on what I was seeing in my debugging session. I saw the first two paragraphs of https://bugzilla.mozilla.org/show_bug.cgi?id=1565542#c4 under the debugger. Thanks!

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

Do we have any thoughts on the relative risk of doing the partial backout from this bug for 68.0.2 vs. forward-fixing via bug 1565542? Note that I'm only worried about the dot release as far as that question is concerned.

This patch will probably bring about the performance regression in bug 1516540 under some circumstances, so I have no desire to land it. :-)

Comment 24

[(no longer active)]

I'm going to mark this as a dupe of bug 1565542, since I don't think there is any reason to keep track of this independently...

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Ehsan, can you verify that today's Nightly builds no longer reproduce?

Comment 26

[(no longer active)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)

Ehsan, can you verify that today's Nightly builds no longer reproduce?

That's right, it doesn't!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Thanks for the confirmation!