1397360 - Fix BrowserUtils.promiseLayoutFlushed to always return a Promise and unregister its callbacks to avoid memory leaks

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, enhancement)

Description

[Mike de Boer [:mikedeboer]]

Also add a non-Promise version to aid in the development of timing-sensitive features, like animations, because using callbacks instead avoids mixing two different clock-based primitives.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1397360 - Fix BrowserUtils.promiseLayoutFlushed to always return a Promise and unregister its callbacks to avoid memory leaks.

I also added a non-Promise version to aid in the development of timing-sensitive
features, like animations, because using callbacks instead avoids mixing two
different clock-based primitives.

Review commit: https://reviewboard.mozilla.org/r/176928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176928/

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8905126 [details]
Bug 1397360 - Fix BrowserUtils.promiseLayoutFlushed to always return a Promise and unregister its callbacks to avoid memory leaks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176928/diff/1-2/

Comment 3

[:Paolo Amadini]

Comment on attachment 8905126 [details]
Bug 1397360 - Fix BrowserUtils.promiseLayoutFlushed to always return a Promise and unregister its callbacks to avoid memory leaks.

The non-Promise-based version doesn't look like something particularly useful. It makes the callback instant or deferred based only on the layout state of the document, meaning that you get synchronous behavior in one case and asynchronous behavior in the other.

Instead, code that relies on waiting for reflow should be written in a way that the wait won't create glitches. In other words, if we replaced the wait on the reflow with a one-second timeout, the user interface should look consistent during that second. If this is not possible, it means that the callers have to be fixed.

Any other solution wouldn't remove the glitches, just make them more intermittent.

Comment 4

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #3)
> If this is not possible, it means that the callers have to be fixed.

...or still use synchronous reflows, like we still do in a few cases already, in which case they won't need to call this API in the first place.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #3)
> The non-Promise-based version doesn't look like something particularly
> useful. It makes the callback instant or deferred based only on the layout
> state of the document, meaning that you get synchronous behavior in one case
> and asynchronous behavior in the other.

Which sounds fine to me! If you know what you're doing, having sync vs. async behavior would be fine. Also note that this is already the case without this patch.
That's also _why_ I wanted to have a non-Promise based alternative, because the two clocks are so different. I'd like to assert that mixing this API with Promises is asking for synchronization issues to occur. Callbacks are not always evil and sometimes even preferred.

> Instead, code that relies on waiting for reflow should be written in a way
> that the wait won't create glitches. In other words, if we replaced the wait
> on the reflow with a one-second timeout, the user interface should look
> consistent during that second. If this is not possible, it means that the
> callers have to be fixed.

I agree, in an ideal world. But that just isn't the case! I'd like to invite you over to try and use the Promise-based version in _any_ of the three callsites and make the animation _not_ glitch. Perhaps I did something very wrong there and if so I'd be very grateful for hints in the right direction :)

> Any other solution wouldn't remove the glitches, just make them more
> intermittent.

Thusly, I disagree, because practice proves to be different for me. But again, I might be doing it wrong and miss something substantial.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> I'd like to invite you over to try and use the Promise-based version in
> _any_ of the three callsites and make the animation _not_ glitch.

Over at bug 1374749, I mean.

Comment 7

[:Paolo Amadini]

Comment on attachment 8905126 [details]
Bug 1397360 - Fix BrowserUtils.promiseLayoutFlushed to always return a Promise and unregister its callbacks to avoid memory leaks.

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Which sounds fine to me! If you know what you're doing, having sync vs.
> async behavior would be fine. Also note that this is already the case
> without this patch.

No, this isn't already the case, because promiseLayoutFlushed is an async function and the Promise it returns will be resolved on the next microtask. I think you missed that this is an async function, and in fact the Promise.resolve you added there has no effect. (I was confused by the subject of the bug, now I realize what you meant.)

> I'd like to invite
> you over to try and use the Promise-based version in _any_ of the three
> callsites and make the animation _not_ glitch. Perhaps I did something very
> wrong there and if so I'd be very grateful for hints in the right direction

Well, I'm reviewing bug 1374749 now and I think an existing issue is that you're setting the "current" attribute too early. Compare it with the non-Photon path, that only does that after the size of the panel and the container has been locked.

This may not be the only issue but may be a good start for you to look into!

Comment 8

[:Paolo Amadini]

Comment on attachment 8905126 [details]
Bug 1397360 - Fix BrowserUtils.promiseLayoutFlushed to always return a Promise and unregister its callbacks to avoid memory leaks.

So I just replaced the calls to wait for a layout flush with a one second timeout as I suggested before, and using this as a baseline I was able to create a forward animation over in bug 1374749 without any glitches.

I think the only bug that has to be fixed here is unregistering the callbacks...

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #7)
> No, this isn't already the case, because promiseLayoutFlushed is an async
> function and the Promise it returns will be resolved on the next microtask.
> I think you missed that this is an async function, and in fact the
> Promise.resolve you added there has no effect. (I was confused by the
> subject of the bug, now I realize what you meant.)

Ah, very true! I missed that as well.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #8)
> So I just replaced the calls to wait for a layout flush with a one second
> timeout as I suggested before, and using this as a baseline I was able to
> create a forward animation over in bug 1374749 without any glitches.

I can't. For me, 1 out of 10 forward transitions is sure to have a glitch.

Comment 11

[Mike de Boer [:mikedeboer]]

Looking more closely, it's in fact impossible for a callback to be called twice, because we stop observing reflows before that - thus there is in fact no leakage of callbacks and this bug can be morphed to my wish to have a non-promise based version of this API:

https://hg.mozilla.org/mozilla-central/file/tip/toolkit/modules/BrowserUtils.jsm#l35

One theoretical case is where callbacks may be called when `reflow` and `reflowInterruptible` are invoked in the same stack. That would dumbfound me, but still.

Comment 12

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Looking more closely, it's in fact impossible for a callback to be called
> twice, because we stop observing reflows before that

Ah, I misread the code. This makes sense, it's basically ensuring we have at most one ReflowObserver created by this module per document.

> - thus there is in fact
> no leakage of callbacks and this bug can be morphed to my wish to have a
> non-promise based version of this API:

Actually this would make it WONTFIX. This hypothetical non-Promise version is _exactly_ the same as doing all the work in the provided callback with the Promise version anyways... and callers shouldn't do any DOM changes in there anyways.

Comment 13

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> I'd like to invite you over to try and use the Promise-based version in _any_ of the three
> callsites and make the animation _not_ glitch.

Given that I accepted the challenge and proved that it's possible, I'm closing this bug ;-)