Created attachment 266220 [details] [review]
background: Fix cancellable issues

If we have the following sequence:

    cache.getImageContent({ filename: "foo", cancellable: cancellable1 });
    cache.getImageContent({ filename: "foo", cancellable: cancellable2 });
    cancellable1.cancel();

Then the second load will complete with "null" as its content, even though
it was never cancelled, and we'll see a blank image. Meanwhile, since the
second load simply appends to the list of callers for the second load,
cancellable2 does absolutely nothing: cancelling it won't stop the load,
and it will still receive onFinished handling.

To prevent this from happening, give the actual load operation its own
Gio.Cancellable, which is "ref-counted" -- only cancel it when all the other
possible callers cancel.

Additionally, clean up the large nested loops by splitting out duplicated
code and other stuff.

Trying this out crashes with:

gnome-shell:31599): Gjs-WARNING **: JS ERROR: TypeError: caller.cancellable is null
BackgroundCache<._loadImageContent@resource:///org/gnome/shell/ui/background.js:202
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
BackgroundCache<.getImageContent@resource:///org/gnome/shell/ui/background.js:261
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
SystemBackground<._init@resource:///org/gnome/shell/ui/background.js:645
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
_Base.prototype._construct@resource:///org/gnome/gjs/modules/lang.js:110
Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/lang.js:204
LayoutManager<._loadBackground@resource:///org/gnome/shell/ui/layout.js:566
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
LayoutManager<.init@resource:///org/gnome/shell/ui/layout.js:252
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
_initializeUI@resource:///org/gnome/shell/ui/main.js:175
start@resource:///org/gnome/shell/ui/main.js:117
@<main>:1

Review of attachment 266220 [details] [review]:

Does not work (a tleast on my setup with two monitors)

Created attachment 269823 [details] [review]
background: Fix cancellable issues

If we have the following sequence:

    cache.getImageContent({ filename: "foo", cancellable: cancellable1 });
    cache.getImageContent({ filename: "foo", cancellable: cancellable2 });
    cancellable1.cancel();

Then the second load will complete with "null" as its content, even though
it was never cancelled, and we'll see a blank image. Meanwhile, since the
second load simply appends to the list of callers for the second load,
cancellable2 does absolutely nothing: cancelling it won't stop the load,
and it will still receive onFinished handling.

To prevent this from happening, give the actual load operation its own
Gio.Cancellable, which is "ref-counted" -- only cancel it when all the other
possible callers cancel.

Additionally, clean up the large nested loops by splitting out duplicated
code and other stuff.

sigh - I want to test this, but it's going to have to wait for the other side of the cogl rebuild.

Looks good in at least one case I've noticed on the fedlet - rotating the screen used to kill the background, now it pops out briefly (probably only noticeable because of a performance bug which makes everything sloooooow on it...) and then comes back quickly. Also have the patches applied on my desktop, will keep an eye out.

Review of attachment 269823 [details] [review]:

LG and does not crash here anymore.

Review of attachment 269823 [details] [review]:

Jasper asked me to look at this, too.  While the patch may fix the bug I think it's a little hard to read.  It does too many things at once:

1) changes the code to always copy the background content instead of only copying it for secondary callers
2) Adds code style changes
3) changes terminology (i.e. pendingFileLoad objects become "info" objects)
4) moves large chunks of the code to a helper function
5) increases code clarity by replacing a nested loop with a call to indexOf

While I think most of those changes are fine in pricinple, they are sort of orthogonal, and make it hard to follow the progression of changes in the patch.

I really recommend splitting them up for future code spelunking sanity.  I actually needed to split the code out to understand what the patch was doing, anyway, so i'll attach what I did here (untested changes).

Created attachment 270420 [details] [review]
background: always copy background content when loading into cache

Copying is actually a lightweight operation, so trying to avoid it just adds
code complexity for little gain.

Based on work from Jasper St. Pierre <jstpierre@macheye.net>

Created attachment 270421 [details] [review]
background: refactor file loading

This commit moves the code around a bit such that the
caller gets allocated up front and then a file load is either
found or created to attach the caller to.

Functionally, the code is the same, it's just now factored in a way
that will make it easier to fix a bug with cancellation later.

Created attachment 270422 [details] [review]
background: move file loading to separate function

This improves readability.

Based on work from Jasper St. Pierre <jstpierre@macheye.net>

Created attachment 270423 [details] [review]
background: get rid of nested loop when finishing file loading

At the moment when a file is loaded, we iterate through the list of
pending file loads and ignore any unrelated to the file, then iterate
all the callers of the related file loads and finish them.

In fact, there can only ever be one pending file load related to the
file, and we already know it, so we can avoid the ugly nested loops.

Created attachment 270424 [details] [review]
background: fix cancellable issue

If we have the following sequence:

    cache.getImageContent({ filename: "foo", cancellable: cancellable1 });
    cache.getImageContent({ filename: "foo", cancellable: cancellable2 });
    cancellable1.cancel();

Then the second load will complete with "null" as its content, even though
it was never cancelled, and we'll see a blank image. Meanwhile, since the
second load simply appends to the list of callers for the second load,
cancellable2 does absolutely nothing: cancelling it won't stop the load,
and it will still receive onFinished handling.

To prevent this from happening, give the actual load operation its own
Gio.Cancellable, which is "ref-counted" -- only cancel it when all the other
possible callers cancel.

Based on work from Jasper St. Pierre <jstpierre@macheye.net>

Review of attachment 270422 [details] [review]:

Hm, is this really necessary? I don't think it helps that much, and I don't see anything in future commits calling finishFileLoad. Can we drop this one?

::: js/ui/background.js
@@ +168,3 @@
+    },
+
+    _finishFileLoad: function(fileLoad) {

I don't like this name. _finish implies some operation has finished, but here, it's just really a continuation of the above function.

@@ +187,3 @@
                                                   let pendingLoad = this._pendingFileLoads[i];
+                                                  if (pendingLoad.filename != filename ||
+                                                      pendingLoad.style != style)

Were these meant to be fileLoad.filename and fileLoad.style?

Review of attachment 270423 [details] [review]:

::: js/ui/background.js
@@ +184,3 @@
                                               }
 
+                                              for (let j = 0; j < fileLoad.callers.length; j++) {

This should probably be renamed to 'i'.

@@ +185,3 @@
 
+                                              for (let j = 0; j < fileLoad.callers.length; j++) {
+                                                  if (fileLoad.callers[j].onFinished) {

We should probably have:

    let caller = fileLoad.callers[i];

Review of attachment 270420 [details] [review]:

OK.

Review of attachment 270421 [details] [review]:

OK.

Review of attachment 270424 [details] [review]:

Yep.

(Chatted with Jasper a bit on IRC, since my earlier comment wasn't too clear)
The patches attached earlier weren't really meant to go to git.  They were just output from me deconstructing attachment 269823 [details] [review]

Marking them needs-work since they're untested, probably have typos, etc.

Created attachment 271614 [details] [review]
background: always copy background content when loading into cache

Copying is actually a lightweight operation, so trying to avoid it just adds
code complexity for little gain.

Based on work from Jasper St. Pierre <jstpierre@macheye.net>

Created attachment 271615 [details] [review]
background: refactor file loading

This commit moves the code around a bit such that the
caller gets allocated up front and then a file load is either
found or created to attach the caller to.

Functionally, the code is the same, it's just now factored in a way
that will make it easier to fix a bug with cancellation later.

Created attachment 271616 [details] [review]
background: get rid of nested loop when finishing file loading

At the moment when a file is loaded, we iterate through the list of
pending file loads and ignore any unrelated to the file, then iterate
all the callers of the related file loads and finish them.

In fact, there can only ever be one pending file load related to the
file, and we already know it, so we can avoid the ugly nested loops.

Created attachment 271617 [details] [review]
background: fix cancellable issue

If we have the following sequence:

    cache.getImageContent({ filename: "foo", cancellable: cancellable1 });
    cache.getImageContent({ filename: "foo", cancellable: cancellable2 });
    cancellable1.cancel();

Then the second load will complete with "null" as its content, even though
it was never cancelled, and we'll see a blank image. Meanwhile, since the
second load simply appends to the list of callers for the second load,
cancellable2 does absolutely nothing: cancelling it won't stop the load,
and it will still receive onFinished handling.

To prevent this from happening, give the actual load operation its own
Gio.Cancellable, which is "ref-counted" -- only cancel it when all the other
possible callers cancel.

Based on work from Jasper St. Pierre <jstpierre@macheye.net>

Review of attachment 271614 [details] [review]:

sure.

Review of attachment 271615 [details] [review]:

k

Review of attachment 271616 [details] [review]:

yup.

Review of attachment 271617 [details] [review]:

++

Attachment 271614 [details] pushed as ec6facb - background: always copy background content when loading into cache
Attachment 271615 [details] pushed as e917b7c - background: refactor file loading
Attachment 271616 [details] pushed as fdf264f - background: get rid of nested loop when finishing file loading
Attachment 271617 [details] pushed as 55d1c7e - background: fix cancellable issue