1224736 - Many UI elements disappear

Status: VERIFIED FIXED

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

Description

[Zéfling]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151113030248

Steps to reproduce:

Reinit profil with a unique installation of Classic Theme Restorer.
(on Nightly, Debian 8 KDE)


Actual results:

Some elements of the interface are graphically unstable, like this :
http://ikilote.net/Galeries/Autres/Divers/Nightly-Glishes.png

Comment 1

[Gingerbread Man]

What version of Classic Theme Restorer are you running? Make sure it's the latest development build.
https://addons.mozilla.org/firefox/addon/classicthemerestorer/versions/

If the issue occurs with the latest development build, report it to the bug tracker, the forum support thread, or contact the author by e-mail (all links on the side of the “About this add-on” box below).
https://addons.mozilla.org/firefox/addon/classicthemerestorer/

Comment 2

[Zéfling]

The last on addons.mozilla.org : 1.4.2
With the same version on Firefox 42.0 & 44.0a2 (2015-11-13) : no problems

Comment 3

[Aris]

Could you please use CTRs support thread or Githubs issue area for reporting issues? Bugzilla is the wrong place for that.

@Admins: please close/delete this bug.

Comment 4

[Zéfling]

I think I found the reason. The profile is corrupted, even after a reset, it does not seem to be better. The most bizarre is that it also breaks applications like Kate or Konsone. When I closed Nightly everything is back to normal. It's really really a strange bug. I delete this profil and make a another.

Comment 5

[Zéfling]

I reopen the bug because the bug persists with a new profile without add-on, with just the import a list ~100 bookmarks et bookmarks panel opened in navigation.

On lunch in console I have this message :

(firefox:3243): Gtk-WARNING **: Locale not supported by C library.
        Using the fallback 'C' locale.
                                                                                                                                                                                                                                
(firefox:3243): Gtk-WARNING **: Locale not supported by C library.
        Using the fallback 'C' locale.
                                                                                                                                                                                                                                
(process:3345): Gtk-WARNING **: Locale not supported by C library.
        Using the fallback 'C' locale.


It's probably a problem with Gtk in KDE. A lot of softwares malfunctions when Nighly is launched.

Comment 6

[Gingerbread Man]

May be a duplicate of bug 1225197.

Comment 7

[u279076]

Could you please do the following:
1) Attach a copy of about:support to this bug report and make sure your screenshot is attached to this bug report as well (URLs have a habit of 404ing)

2) If you didn't get Nightly from nightly.mozilla.org please do so and retest. If it still happens, try a GTK session to see if this is isolated to KDE.

3) Check if this happens with a 44.0a1 Nightly (available from archive.mozilla.org) and if it doesn't then please use mozregression to track down the regression range.

4) Finally, please check to see if there is a similar issue reported in the KDE project, your Linux distribution, and/or the user forums for each. If this is not a Firefox bug then you may find workarounds there.

Thanks

Comment 8

[Zéfling]

Curently, the last Nightly is the only soft problematic to the point of having to reboot. Firefox 44.0a2 (2015-11-19) does not create a problem. 45.0a1 (2015-11-19) is not stable.
I look for a similar bug for KDE, but I have not found.


OS version : 
- Linux debian-Z1 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt11-1+deb8u6 (2015-11-09) x86_64 GNU/Linux 
8.2
- Debian GNU/Linux 8

KDE version : 
- Qt : 4.8.6
- KDE : 4.14.2
- KIO Client : 2.0

Firefox Nightly : 
Paramètres de base de l'application
-----------------------------------

Nom: Firefox
Version: 45.0a1
Identifiant de compilation: 20151119065326
Canal de mise à jour: nightly
Agent utilisateur: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Fenêtres multi-processus: 1/1 (par défaut : true)
Mode sans échec: false

Rapports de plantage des 3 derniers jours
-----------------------------------------

Tous les rapports de plantage

Extensions
----------

Accélération graphique
----------------------

Description de la carte: NVIDIA Corporation -- GeForce GTX 770/PCIe/SSE2
Fenêtres avec accélération graphique: 0/1 Basic (OMTC)
ID du périphérique: GeForce GTX 770/PCIe/SSE2
ID du vendeur: NVIDIA Corporation
Prise en charge matérielle pour le décodage H264: No;
Rendu WebGL: NVIDIA Corporation -- GeForce GTX 770/PCIe/SSE2
Version du pilote: 4.4.0 NVIDIA 340.65
windowLayerManagerRemote: true
Zoom/Panoramique asynchrones: entrée molette activée
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
AzureSkiaAccelerated: 0
CairoUseXRender: 1

Préférences modifiées importantes
---------------------------------

accessibility.typeaheadfind.flashBar: 0
browser.cache.disk.capacity: 358400
browser.cache.disk.filesystem_reported: 1
browser.cache.disk.smart_size.first_run: false
browser.cache.frecency_experiment: 1
browser.download.importedFromSqlite: true
browser.places.smartBookmarksVersion: 7
browser.sessionstore.upgradeBackup.latestBuildID: 20151119065326
browser.startup.homepage_override.buildID: 20151119065326
browser.startup.homepage_override.mstone: 45.0a1
dom.apps.reset-permissions: true
dom.mozApps.used: true
extensions.lastAppVersion: 45.0a1
media.gmp-gmpopenh264.abi: x86_64-gcc3
media.gmp-gmpopenh264.lastUpdate: 1447879754
media.gmp-gmpopenh264.version: 1.5.1
media.gmp-manager.buildID: 20151118030232
media.gmp-manager.lastCheck: 1447879754
network.cookie.prefsMigrated: true
network.predictor.cleaned-up: true
places.history.expiration.transient_current_max_pages: 90476
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
privacy.sanitize.migrateClearSavedPwdsOnExit: true

Préférences verrouillées importantes
------------------------------------

JavaScript
----------

Ramasse-miettes incrémentiel: true

Accessibilité
-------------

Activée: false
Empêcher l'accessibilité: 0

Versions des bibliothèques
--------------------------

NSPR
Version minimale attendue: 4.11 Beta
Version utilisée: 4.11 Beta

NSS
Version minimale attendue: 3.21 Basic ECC
Version utilisée: 3.21 Basic ECC

NSSSMIME
Version minimale attendue: 3.21 Basic ECC
Version utilisée: 3.21 Basic ECC

NSSSSL
Version minimale attendue: 3.21 Basic ECC
Version utilisée: 3.21 Basic ECC

NSSUTIL
Version minimale attendue: 3.21
Version utilisée: 3.21

Fonctionnalités expérimentales
------------------------------

Bac à sable
-----------

Seccomp-BPF (Filtrage des appels système): true
Synchronisation du fil d'exécution Seccomp: false
Espace de noms utilisateur pour les processus privilégiés: true
Espace de noms utilisateur: false
Bac à sable pour les plugins multimédia: true

Comment 9

[u279076]

Thanks it looks like you're using the proprietary driver. Does this reproduce with the open-source driver?

Comment 10

[Zéfling]

Exactly same problem with nouveau :

Paramètres de base de l'application
-----------------------------------

Nom: Firefox
Version: 45.0a1
Identifiant de compilation: 20151121030232
Canal de mise à jour: nightly
Agent utilisateur: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Fenêtres multi-processus: 1/1 (par défaut : true)
Mode sans échec: false

Rapports de plantage des 3 derniers jours
-----------------------------------------

Tous les rapports de plantage

Extensions
----------

Accélération graphique
----------------------

Description de la carte: nouveau -- Gallium 0.4 on NVE4
Fenêtres avec accélération graphique: 0/1 Basic (OMTC)
ID du périphérique: Gallium 0.4 on NVE4
ID du vendeur: nouveau
Prise en charge matérielle pour le décodage H264: No;
Rendu WebGL: nouveau -- Gallium 0.4 on NVE4
Version du pilote: 3.0 Mesa 10.3.2
windowLayerManagerRemote: true
Zoom/Panoramique asynchrones: entrée molette activée
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
AzureSkiaAccelerated: 0
CairoUseXRender: 1

Préférences modifiées importantes
---------------------------------

accessibility.typeaheadfind.flashBar: 0
browser.cache.disk.capacity: 358400
browser.cache.disk.filesystem_reported: 1
browser.cache.disk.smart_size.first_run: false
browser.cache.frecency_experiment: 1
browser.download.importedFromSqlite: true
browser.places.smartBookmarksVersion: 7
browser.sessionstore.upgradeBackup.latestBuildID: 20151121030232
browser.startup.homepage_override.buildID: 20151121030232
browser.startup.homepage_override.mstone: 45.0a1
dom.apps.reset-permissions: true
dom.mozApps.used: true
extensions.lastAppVersion: 45.0a1
gfx.crash-guard.glcontext.appVersion: 44.0a2
gfx.crash-guard.glcontext.deviceID: GeForce GTX 770/PCIe/SSE2
gfx.crash-guard.glcontext.driverVersion: 4.4.0 NVIDIA 340.65
gfx.crash-guard.status.glcontext: 2
media.gmp-gmpopenh264.abi: x86_64-gcc3
media.gmp-gmpopenh264.lastUpdate: 1447879754
media.gmp-gmpopenh264.version: 1.5.1
media.gmp-manager.buildID: 20151119065326
media.gmp-manager.lastCheck: 1448142586
network.cookie.prefsMigrated: true
network.predictor.cleaned-up: true
places.database.lastMaintenance: 1448142819
places.history.expiration.transient_current_max_pages: 53005
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
privacy.sanitize.migrateClearSavedPwdsOnExit: true
storage.vacuum.last.index: 0
storage.vacuum.last.places.sqlite: 1448142819

Préférences verrouillées importantes
------------------------------------

JavaScript
----------

Ramasse-miettes incrémentiel: true

Accessibilité
-------------

Activée: false
Empêcher l'accessibilité: 0

Versions des bibliothèques
--------------------------

NSPR
Version minimale attendue: 4.11
Version utilisée: 4.11

NSS
Version minimale attendue: 3.21 Basic ECC
Version utilisée: 3.21 Basic ECC

NSSSMIME
Version minimale attendue: 3.21 Basic ECC
Version utilisée: 3.21 Basic ECC

NSSSSL
Version minimale attendue: 3.21 Basic ECC
Version utilisée: 3.21 Basic ECC

NSSUTIL
Version minimale attendue: 3.21
Version utilisée: 3.21

Fonctionnalités expérimentales
------------------------------

Bac à sable
-----------

Seccomp-BPF (Filtrage des appels système): true
Synchronisation du fil d'exécution Seccomp: false
Espace de noms utilisateur pour les processus privilégiés: true
Espace de noms utilisateur: false
Bac à sable pour les plugins multimédia: true

Comment 11

[u279076]

Thanks, can you check if this reproduces in a GTK session?

Comment 12

[Zéfling]

What do you mean by session GTK with KDE ?

Comment 13

[u279076]

(In reply to Zéfling from comment #12)
> What do you mean by session GTK with KDE ?

Sorry for being unclear. I meant to run a Gnome Shell session on the same machine and see if it reproduces. This is necessary to confirm if KDE is a factor or not.

Comment 14

[Zéfling]

With Gnome, same probleme......

I don't know if it's the good version (installed with : sudo apt-get install gnome-shell gnome-shell-extensions-* gnome-tweak-tool gnome-documents gnome-sushi).

Comment 15

[u279076]

Thanks, this at least confirms it's not an issue specific to KDE. Could you please run the mozregression tool to narrow down when this started? http://mozilla.github.io/mozregression/install.html

Comment 16

[Zéfling]

Thank's for mozregression, :)

It's between nightlies : 2015-10-28 and 2015-10-29

My test (no profil) :
- import a big list of bookmarks
- open bookmarks panel
- scroll in bookmarks panel
It's bad when the list of bookmark is not displaying correctly.

Comment 17

[u279076]

It should have given you a pushlog URL, something starting with hg.mozilla.org/...

Comment 18

[Zéfling]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fc706d376f0658e560a59c3dd520437b18e8c4a4&tochange=acb3f4ac5424181d3d4d73283668162137918cf1

Comment 19

[u279076]

(In reply to Zéfling from comment #18)
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=fc706d376f0658e560a59c3dd520437b18e8c4a4&tochange=acb3
> f4ac5424181d3d4d73283668162137918cf1

A lot of changes in there but most notable is the switch-over to GTK3. I'm wondering if you could bisect this further to mozilla-inbound with mozregression?

Comment 20

[u279076]

Mike, any chance comment 19 is true?

Comment 21

[Zéfling]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=87b8ec6a9c72ae5e0b71260af51a6de2be38f5de&tochange=9a2aa5e4cde39a7a8b726011bf6ea8b2c22c2ae3

Comment 22

[Mike Hommey [:glandium]]

An easy way to check is to download an elm build and see what happens with it.

http://archive.mozilla.org/pub/firefox/tinderbox-builds/elm-linux64/1447808077/firefox-45.0a1.en-US.linux-x86_64.tar.bz2

Comment 23

[Zéfling]

This version do not work correctly.

Comment 24

[Mike Hommey [:glandium]]

(In reply to Zéfling from comment #23)
> This version do not work correctly.

If you mean the one from comment 22, then that means Gtk+3 is not involved. I guess comment 21 is the most relevant information now.

Comment 25

[Mike Hommey [:glandium]]

Daniel, could you take a look at this, since you authored bug 1218041

Comment 26

[Daniel Holbert [:dholbert]]

Do we have reliable Steps To Reproduce here? I don't see any issues with just Classic Theme Restorer add-on installed. I tried bookmarking a few sites that have SVG favicons, too.

Sounds like importing a particular set of 100 bookmarks might reproduce this, per comment 5 - Zéfling, could you either post that list of bookmarks? (and/or see if you can narrow down a subset of that list -- maybe just a single site -- which reproduces the bug?)

Comment 27

[Zéfling]

Attachment: The problematic bookmark

I found my killer Firefox bookmark :)

Comment 28

[Daniel Holbert [:dholbert]]

Thanks. I'm still having trouble reproducing, though.  Can you reproduce this bug if you simply visit that site in a fresh profile, and bookmark it?

It doesn't even show a favicon for me (or have one in its source AFAICT), which I expect would be the piece that'd be tripping us up here.  (Perhaps it used to show a favicon, and that's saved in your favicon cache in your profile, though?)

Comment 29

[Zéfling]

I thinks the problem is with this correct image : 

ICON="data:image/png;base64,PCFET0NUWVBFIGh0bWw+CjxodG1sIGxhbmc9ZW4+CiAgPG1ldGEgY2hhcnNldD11dGYtOD4KICA8bWV0YSBuYW1lPXZpZXdwb3J0IGNvbnRlbnQ9ImluaXRpYWwtc2NhbGU9MSwgbWluaW11bS1zY2FsZT0xLCB3aWR0aD1kZXZpY2Utd2lkdGgiPgogIDx0aXRsZT5FcnJvciA0MDQgKE5vdCBGb3VuZCkhITE8L3RpdGxlPgogIDxzdHlsZT4KICAgICp7bWFyZ2luOjA7cGFkZGluZzowfWh0bWwsY29kZXtmb250OjE1cHgvMjJweCBhcmlhbCxzYW5zLXNlcmlmfWh0bWx7YmFja2dyb3VuZDojZmZmO2NvbG9yOiMyMjI7cGFkZGluZzoxNXB4fWJvZHl7bWFyZ2luOjclIGF1dG8gMDttYXgtd2lkdGg6MzkwcHg7bWluLWhlaWdodDoxODBweDtwYWRkaW5nOjMwcHggMCAxNXB4fSogPiBib2R5e2JhY2tncm91bmQ6dXJsKC8vd3d3Lmdvb2dsZS5jb20vaW1hZ2VzL2Vycm9ycy9yb2JvdC5wbmcpIDEwMCUgNXB4IG5vLXJlcGVhdDtwYWRkaW5nLXJpZ2h0OjIwNXB4fXB7bWFyZ2luOjExcHggMCAyMnB4O292ZXJmbG93OmhpZGRlbn1pbnN7Y29sb3I6Izc3Nzt0ZXh0LWRlY29yYXRpb246bm9uZX1hIGltZ3tib3JkZXI6MH1AbWVkaWEgc2NyZWVuIGFuZCAobWF4LXdpZHRoOjc3MnB4KXtib2R5e2JhY2tncm91bmQ6bm9uZTttYXJnaW4tdG9wOjA7bWF4LXdpZHRoOm5vbmU7cGFkZGluZy1yaWdodDowfX0KICA8L3N0eWxlPgogIDxhIGhyZWY9Ly93d3cuZ29vZ2xlLmNvbS8+PGltZyBzcmM9Ly93d3cuZ29vZ2xlLmNvbS9pbWFnZXMvZXJyb3JzL2xvZ29fc20uZ2lmIGFsdD1Hb29nbGU+PC9hPgogIDxwPjxiPjQwNC48L2I+IDxpbnM+VGhhdOKAmXMgYW4gZXJyb3IuPC9pbnM+CiAgPHA+VGhlIHJlcXVlc3RlZCBVUkwgPGNvZGU+L2Zhdmljb24uaWNvPC9jb2RlPiB3YXMgbm90IGZvdW5kIG9uIHRoaXMgc2VydmVyLiAgPGlucz5UaGF04oCZcyBhbGwgd2Uga25vdy48L2lucz4K"

The website has no image. This is probably the favicon cache that breaks interface. It is an old bookmark and I will remove. But it's impressive the damage that it makes on Firefox.

Comment 30

[Zéfling]

Oups, is not "correct" but "corrupt".

Comment 31

[alta88]

This problem is also showing up in Thunderbird when rendering a longish (scrollable) nsITreeView full of feed folder favicons, similar to Library or History Sidebar in Fx (but I don't see this in History Sidebar favicons and don't have many bookmarks in the trunk test profile). These favicon urls are mostly .ico or .png extensions. The scroll perf is degraded and the tree does not completely repaint until mouseover, and seeming resolution of all the icons in the visible rows.  Tb uses setAndFetchFaviconForPage() and after a delay checks the isFailedFavicon() cache; if the url is not there it is returned in getImageSrc().

Last good was Tb nightly on 10/28, first bad is the build of 10/29.  I also see the problem in bug 1225197 in Fx nightly menuitems.

Comment 32

[Daniel Holbert [:dholbert]]

Zéfling, any chance you'd be willing to zip up a minimal-ish Firefox profile which triggers this bug and email it to me?

I've tried several ways of triggering the bug with the image data-URI in comment 29, as well as with the bookmark from comment 27, and I'm still unable to reproduce.

Comment 33

[Zéfling]

Attachment: profil.tar.gz

Comment 34

[Daniel Holbert [:dholbert]]

Thanks very much!  I can reproduce the bug with latest nightly & that profile, in an Ubuntu VM.  (Specifically: the lower half of the left bookmarks sidebar doesn't render in that profile [though pieces will flicker into view when I mouseover that area]).  The missing area *does* render if I delete the (blank-title) webglsamples history-item, though -- the first line at the top of the blank area.

I'm catching up on code-reviews after the Mozilla work week, and then I'll take a closer look with a debugger.

Comment 35

[Daniel Holbert [:dholbert]]

FACTS:
 * The patch that regressed this was: https://hg.mozilla.org/integration/mozilla-inbound/rev/625650d1493c
 * When we hit that code in Zefling's profile, we've got a broken RasterImage object (which fails when we ask it for GetWidth/GetHeight because it has mError = true)

 * IN THE OLD CODE (replaced in 625650d1493c):
   - GetWidth/GetHeight would fail, which gave us a 0-sized rawImageCSSIntSize as well as a 0-sized sourceRect.
   - We'd call GetWholeImageDestination to divide those variables (and do some divide-by-0) to produce a wholeImageDest with all of its member-variables set to huge negative numbers.
   - A bit later, we try to draw via DrawImageInternal, but we bail early because ComputeSnappedImageDrawingParameters detects that our destination area (from wholeImageDest in the patch above) is negative-sized. Phew! So no drawing actually happens for this image, thanks to our division-by-0-producing-huge-negative-numbers.

 * IN THE NEW CODE (added in 625650d1493c):
   - We set up a sane (for SVG at least) "wholeImageDest" rect, equal to the passed-in dest rect.  Hooray, no division by 0 and no huge negative numbers.
   - We get a bit further in ComputeSnappedImageDrawingParameters, and call OptimalImageSizeForDest() on our image-that's-in-an-error-state, and unsurprisingly it returns 0,0.
   - This makes us produce a 0-sized |subimage| rect, which produces an invalid matrix when we call TransformBetweenRects:
(gdb) p transform
$52 = {
  _11 = inf, 
  _12 = 0, 
  _21 = 0, 
  _22 = inf, 
  _31 = -nan(0x8000000000000), 
  _32 = -nan(0x8000000000000)
}
   - I didn't trace in detail past this point, but presumably we use this invalid matrix for drawing, and it busts our internal Cairo state, because I see stuff like this logged to my terminal (in a debug build):
[GFX2-]: ASurface Init failed with Cairo status 1 on 0x0x7fb71d901ba8
[GFX2-]: DrawTargetCairo context in error state: invalid matrix (not invertible)(5)

So bottom-line, we need to special-case bug 1218041's "zero-size means we draw to the full dest area" business to ONLY happen if the image reports a status that indicates that it's ready to draw. Otherwise, we should leave wholeImageDest as being an empty rect, to prevent attempts at drawing. (That will make us take ComputeSnappedImageDrawingParameters's early-return, if it doesn't make us return earlier.)

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This adds a special case as described at the end of my previous comment.

Two final notes here:
 (1) For a scenario where we've got a busted RasterImage (with mError == true), this patch will make us do fewer Bad Things than we did before this bug regressed, and will restore the old (expected) behavior, in the following ways:
 - We won't divide by 0 anymore (Bad Thing) when trying to compute wholeImageDest (as we used to do, per comment 35).
 - So, wholeImageDest ends up being left at its default 0-size instead of having a huge bogus negative number as its size (another Bad Thing).
 - This 0 size is still sufficient to get us to bail early from ComputeSnappedImageDrawingParameters, which makes us bail early from DrawImageInternal, which makes us not paint anything for busted images (and avoid messing up the internal Cairo state as we did in this bug here). So, we'll get the old behavior back. Hooray!

 (2) Busted SVG images won't mess up the internal cairo transform in the way that busted-RasterImages do, because they won't have this particular piece of fatal behavior that I'll quote from comment 35:
>    - We ... call
> OptimalImageSizeForDest() on our image-that's-in-an-error-state, and
> unsurprisingly it returns 0,0.
This works out better for VectorImage, because its OptimalImageSizeForDest implementation just returns the passed-in Dest size -- not 0,0 like busted-RasterImages do. So VectorImage won't end up producing a broken transform via division-by-0 like busted RasterImages currently do (as described in comment 35).

Comment 37

[Daniel Holbert [:dholbert]]

One more note:
 (3) I initially thought about checking the image's status. However, I believe we need the imgIRequest in order to query the image status, and we don't have the imgIRequest pointer in scope.  We could look it up, and then look up the status, but that (combined with the VectorImage check) makes this logic probably more complex than it needs to be.

Comment 38

[Daniel Holbert [:dholbert]]

(Also: I'm not going to invest time in coming up with a reftest for this bug at this point, given that we can't reftest these elements at all right now anyway, due to bug 1218954. And I had some trouble reproducing this in a standalone test, per comment 32, which leads me to believe a working reftest may be tricky even in the absence of bug 1218954.)

Comment 40

[Daniel Holbert [:dholbert]]

Seth, gentle review-ping (or please let me know if I should tag :tn or someone else to review this).  I'd love to get this fixed & uplifted to Aurora before Firefox 45 moves to Beta.

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8699373 [details] [diff] [review]
fix v1

Just talked this over with :tn on IRC, actually; I'll just go ahead and transfer r? to him.

Comment 42

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8699373 [details] [diff] [review]
fix v1

(In reply to Daniel Holbert [:dholbert] from comment #35)
>  * IN THE NEW CODE (added in 625650d1493c):
>    - We set up a sane (for SVG at least) "wholeImageDest" rect, equal to the
> passed-in dest rect.  Hooray, no division by 0 and no huge negative numbers.
>    - We get a bit further in ComputeSnappedImageDrawingParameters, and call
> OptimalImageSizeForDest() on our image-that's-in-an-error-state, and
> unsurprisingly it returns 0,0.
>    - This makes us produce a 0-sized |subimage| rect, which produces an
> invalid matrix when we call TransformBetweenRects:
> (gdb) p transform
> $52 = {
>   _11 = inf, 
>   _12 = 0, 
>   _21 = 0, 
>   _22 = inf, 
>   _31 = -nan(0x8000000000000), 
>   _32 = -nan(0x8000000000000)
> }

As discussed on irc, ComputeSnappedImageDrawingParameters should probably be able to handle this situation without putting our drawing context into a broken state. So we should have a followup bug for that (unless you want to do that here too).

Comment 43

[Daniel Holbert [:dholbert]]

Attachment: part 2: Handle this gracefully in ComputeSnappedImageDrawingParameters

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 8706655 [details] [diff] [review]
part 2: Handle this gracefully in ComputeSnappedImageDrawingParameters

Here's the change discussed on IRC - we should be robust against the sort of scenario described in comment 35, and stop ourselves from generating a bogus transform & using it.

This patch does that by testing |subimage| for being zero-sized.  (Further down, we divide by subimage's width & height via a call to TransformBetweenRects, which means divide-by-0 in this bug as described in comment 35.)

I've verified locally that this patch, in isolation, is sufficient to fix this bug's symptom, with the attached testing profile tarball.

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 8706655 [details] [diff] [review]
part 2: Handle this gracefully in ComputeSnappedImageDrawingParameters

Actually, yeah -- I'll spin off a followup bug for "part 2", because I'd like to backport the fix for this bug (part 1) but I don't think it's worth bothering to backport "part 2" (minor risk of breakage).

Comment 46

[Daniel Holbert [:dholbert]]

I filed bug 1238777 as the followup to comment 42. (I originally attached the patch here, but I've now marked those comments as obsolete.)

Thanks for the review!

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28b9bbf643e6

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28b9bbf643e6

Comment 49

[alta88]

Confirming that this also fixes the Tb problem, thanks!.  It would be really great to get this onto Fx45 from which the next Tb release will be built.

Comment 50

[Daniel Holbert [:dholbert]]

Comment on attachment 8699373 [details] [diff] [review]
fix v1

Great, thanks for the confirmation! Yeah, let's get this backported to 45. Requesting Aurora approval.

Approval Request Comment
[Feature/regressing bug #]: bug 1218041

[User impact if declined]: pieces of Firefox UI (toolbar icons, bookmark favicons) may not paint, if a broken image is present in the favicon cache & we try to draw that image (e.g. in bookmarks/history sidebar)

[Describe test coverage new/current, TreeHerder]: Sadly, none; this is in code that is hard to test reliably, per bug 1218954. And on top of that, this particular bug is difficult to trigger in a testcase, as noted in comment 38.

[Risks and why]: Low-risk -- this just restricts the code from regressing-bug 1218041 to *only* take effect for SVG images. So this returns us to behaving as we did before the regressing patch landed, for non-SVG images (like the broken favicon-cache images that cause this bug).

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

Comment 51

[Daniel Holbert [:dholbert]]

Actually, after looking at regressing bug 1218041, I realized this affects Firefox 44 too! (Just tested locally with attached test-profile, and confirmed that it's affected.)  Updating firefox44 status here. (I think I had set status for 44 to "unaffected" based on comment 2, but I suspect that comment 2 was posted using a build before the regression landed, or something.)

So, I'll request beta approval as well. Fortunately, the patch is very targeted & returns us to being in a state that we've shipped before, so very low-risk.

Comment 52

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Verified based on comment 49.

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Zefling, could you please verify this issue is fixed as expected on a latest Nightly build (1/12/16)? Thanks!

Comment 54

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8699373 [details] [diff] [review]
fix v1

The bar for Beta44 uplifts is very high and this fix meets the bar for the following reasons:
1. This is a new regression in Fx44.
2. Once an end-user gets into this state, tools and bookmarks menu will continue to get painted incorrectly with missing images and there is no easy way to fix it.
3. The patch is a one-liner that amounts to a partial back out.
4. Verified by TB team.

Given the very low risk, let's uplift to Beta44, Aurora45

Comment 55

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb11e926ac33
https://hg.mozilla.org/releases/mozilla-beta/rev/94987e23aa72

Comment 56

[Zéfling]

It's ok for me with nightly 2016-01-12.

Comment 57

[Daniel Holbert [:dholbert]]

Thanks, Zéfling! --> Marking "verified" for Firefox 46.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/94987e23aa72

Comment 59

[Paul Silaghi, QA [:pauly]]

I've reproduced the problem with the attached profile on Nightly 2015-12-15, Ubuntu 14.04 x64.
Verified fixed FF 44b9, 45.0a2(2016-01-17).