#249 Stabilize and enable intersectionObservers

Closed
opened 2 years ago by wolfbeast · 14 comments
wolfbeast commented 2 years ago (Migrated from github.com)

Painted into a corner, but this should work.

Intersections have been moved to be handled with display lists in FF 18. Mostly because a per-update check of visibility is disastrous for some corner cases (it seems to be the more logical approach to me, but hey...).

As a result off-screen animations will be updated and painted. This can take 100% CPU in some cases (e.g. Facebook loading throbber). So, the display lists need an intersection check anyway in the form of an intersectionObserver.

We have the base code for it, but it’s crashy. (damn hash tables again).
So... Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1322717 and we can then likely enable this for UXP.

Painted into a corner, but this should work. Intersections have been moved to be handled with display lists in FF 18. Mostly because a per-update check of visibility is disastrous for some corner cases (it seems to be the more logical approach to me, but hey...). As a result off-screen animations will be updated and painted. This can take 100% CPU in some cases (e.g. Facebook loading throbber). So, the display lists need an intersection check anyway in the form of an intersectionObserver. We have the base code for it, but it's crashy. (damn hash tables again). So... Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1322717 and we can then likely enable this for UXP.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Looks like there’s a bit more crashiness to be solved.

Looks like there's a bit more crashiness to be solved.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

This is still disabled after issues with the unstable spec and Chromium-based implementation preferences at the time with breaking changes. We should revisit this, see what the spec has settled on now, and implement that, then flip it back on. If big sites still have issues with it we’ll have to either point them to the spec or adjust where needed if we’re still non-compliant.

This is still disabled after issues with the unstable spec and Chromium-based implementation preferences at the time with breaking changes. We should revisit this, see what the spec has settled on now, and implement that, then flip it back on. If big sites still have issues with it we'll have to either point them to the spec or adjust where needed if we're still non-compliant.
Sa-Ja-Di commented 2 years ago (Migrated from github.com)
Owner

I would say this one still leads to crashes. Before i have updated to the unstable with this enabled, i never experienced a crash here:

https://www.robtex.com/ip-lookup/5.79.119.119

Getting random crashes when opening/closing such robtex pages with the 64 Bit unstable. Deactivated now

dom.IntersectionObserver.enabled

and the crashes are gone again.

I would say this one still leads to crashes. Before i have updated to the unstable with this enabled, i never experienced a crash here: https://www.robtex.com/ip-lookup/5.79.119.119 Getting random crashes when opening/closing such robtex pages with the 64 Bit unstable. Deactivated now dom.IntersectionObserver.enabled and the crashes are gone again.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Yup, it does, and the cause is Mozilla’s fucking hashtables again. I’m getting really tired of the crashiness of those things.
At least the crash is easy to track. I’d honestly rather deal with crashes than things “not working as they are supposed to”.

Yup, it does, and the cause is Mozilla's fucking hashtables again. I'm getting really tired of the crashiness of those things. At least the crash is easy to track. I'd honestly rather deal with crashes than things "not working as they are supposed to".
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Latest unstable should have this fixed with commit 3cf7e874f

Latest unstable should have this fixed with commit 3cf7e874f
kn-yami commented 2 years ago (Migrated from github.com)
Owner

Is isIntersecing implemented in UXP?

Is [isIntersecing](https://www.w3.org/TR/intersection-observer/#dom-intersectionobserverentry-isintersecting) implemented in UXP?
JustOff commented 2 years ago (Migrated from github.com)
Owner

It seem this still breaks sites in the wild, see https://forum.palemoon.org/viewtopic.php?p=159684#p159684.

It seem this still breaks sites in the wild, see https://forum.palemoon.org/viewtopic.php?p=159684#p159684.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

The issue there is a spec change that added isIntersecting as a property, because apparently it’s too difficult to understand for webmasters that 0% intersection equals not intersecting. 😝

The issue there is a spec change that added `isIntersecting` as a property, because apparently it's too difficult to understand for webmasters that 0% intersection equals not intersecting. :stuck_out_tongue_closed_eyes:
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Is isIntersecing implemented in UXP?

I completely didn’t see this. Sorry.

I guess the answer is: “it is now!” 😀

> > > Is [isIntersecing](https://www.w3.org/TR/intersection-observer/#dom-intersectionobserverentry-isintersecting) implemented in UXP? I completely didn't see this. Sorry. I guess the answer is: "it is now!" :grinning:
JustOff commented 2 years ago (Migrated from github.com)
Owner

As far as I can see f6ef8d8ca7 resolved the https://forum.palemoon.org/viewtopic.php?p=159684 issue 👍

As far as I can see https://github.com/MoonchildProductions/UXP/commit/f6ef8d8ca7ed96d699c28914fc590b0604520fd0 resolved the https://forum.palemoon.org/viewtopic.php?p=159684 issue :+1:
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

I’m going to disable IntersectionObservers in 28.3.1 because it’s causing too many crashes and crash-evaluation is currently difficult.
Potentially, if the crashes (seems like a CC problem) can be resolved, they can be re-enabled in 28.4, but it’ll need some time to be properly checked on various sites. Keeping it enabled on master so the unstable channel can be a testbed.

I'm going to disable IntersectionObservers in 28.3.1 because it's causing too many crashes and crash-evaluation is currently difficult. Potentially, if the crashes (seems like a CC problem) can be resolved, they can be re-enabled in 28.4, but it'll need some time to be properly checked on various sites. Keeping it enabled on master so the unstable channel can be a testbed.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Clearing milestone for this and putting it on hold -- I’ve been unable to find a solution for #934 and #935 so far and have run out of avenues to explore. I’m probably missing something blatantly simple but can’t see it.

Considering this feature is WD status, experimental, and so far only being used for less than user-interest focused purposes (tracking, ad-visibility flags, potential profiling by observing scroll position, etc.), it’s not a great loss.

This will be disabled by default on master, also.

Clearing milestone for this and putting it on hold -- I've been unable to find a solution for #934 and #935 so far and have run out of avenues to explore. I'm probably missing something blatantly simple but can't see it. Considering this feature is WD status, experimental, and so far only being used for less than user-interest focused purposes (tracking, ad-visibility flags, potential profiling by observing scroll position, etc.), it's not a great loss. This will be disabled by default on master, also.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Looks like I’ve worked around both crashes now, so this can be released again.

Looks like I've worked around both crashes now, so this can be released again.
wolfbeast commented 2 years ago (Migrated from github.com)
Owner

Both crashes are verified fixed, considering this solved and safe to keep re-enabled

Both crashes are verified fixed, considering this solved and safe to keep re-enabled
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.