Skip to content

Conversation

@implicitfield
Copy link
Contributor

@implicitfield implicitfield commented Oct 25, 2025

PRs cherry-picked:
LadybirdBrowser/ladybird#303 (except for the first commit)
LadybirdBrowser/ladybird#277
LadybirdBrowser/ladybird#386
LadybirdBrowser/ladybird#664
LadybirdBrowser/ladybird#1029
LadybirdBrowser/ladybird#929 (except for the last commit; the test was flaky, took a while to run, and was eventually removed along with the rest of LibThreading)
LadybirdBrowser/ladybird#1265 (manually rebaselined)
LadybirdBrowser/ladybird#1308 (manually rebaselined)

Commits cherry-picked:
be2c484
e37071a
c92f8ab
c62cc91
eba33b9
877adcc
35f4b8b
e45b7b0
cbf1fd3

Also added more PRs and commits to Meta/lb-cherry-picks.py again (and fixed up a mistake from last time).

I feel like at this point the main thing that's blocking a lot of cherry-picks from LibWeb is the DisplayListPlayer{GPU,CPU} situation, with neither of those existing upstream anymore. I guess we'd need to mirror changes made to DisplayListPlayerSkia in the players that we do have (though the GPU-accelerated player is already somewhat limited as-is) in order to properly cherry-pick any of the more involved painting-related commits (or those that depend on such commits; this has pretty heavy knock-on effects).

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 25, 2025
@implicitfield implicitfield force-pushed the cherry-picks branch 3 times, most recently from 239bb02 to 73a3a1b Compare October 25, 2025 21:19
@implicitfield
Copy link
Contributor Author

From CI:

571/1489: Layout/i/home/runner/work/serenity/serenity/Meta/Lagom/../../AK/RefCounted.h:60:37: runtime error: downcast of address 0x7c49479e9880 which does not point to an object of type 'const Core::EventReceiver'
0x7c49479e9880: note: object has invalid vptr
 01 00 00 00  87 01 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
    #0 0x7f89498f19e9 in AK::RefCounted<Core::EventReceiver>::unref() const /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/RefCounted.h:60:37
    #1 0x7f89498f6a9e in unref_if_not_null<Core::DeferredInvocationContext> /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/NonnullRefPtr.h:32:14
    #2 0x7f89498f6a9e in ~NonnullRefPtr /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/NonnullRefPtr.h:97:9
    #3 0x7f89498f6a9e in ~DeferredInvocationEvent /home/runner/work/serenity/serenity/Meta/Lagom/../../Userland/Libraries/LibCore/Event.h:50:7
    #4 0x7f89498f6a9e in Core::DeferredInvocationEvent::~DeferredInvocationEvent() /home/runner/work/serenity/serenity/Meta/Lagom/../../Userland/Libraries/LibCore/Event.h:50:7
    #5 0x7f89499189f3 in clear /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/NonnullOwnPtr.h:133:9
    #6 0x7f89499189f3 in ~NonnullOwnPtr /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/NonnullOwnPtr.h:50:9
    #7 0x7f89499189f3 in Core::ThreadEventQueue::Private::QueuedEvent::~QueuedEvent() /home/runner/work/serenity/serenity/Userland/Libraries/LibCore/ThreadEventQueue.cpp:31:32
    #8 0x7f894991a055 in clear_with_capacity /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/Vector.h:402:24
    #9 0x7f894991a055 in AK::Vector<Core::ThreadEventQueue::Private::QueuedEvent, 0ul>::clear() /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/Vector.h:391:9
    #10 0x7f8949916fda in ~Vector /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/Vector.h:110:9
    #11 0x7f8949916fda in Core::ThreadEventQueue::process() /home/runner/work/serenity/serenity/Userland/Libraries/LibCore/ThreadEventQueue.cpp:134:1
    #12 0x7f89498f7870 in Core::EventLoopImplementationUnix::exec() /home/runner/work/serenity/serenity/Userland/Libraries/LibCore/EventLoopImplementationUnix.cpp:316:9
    #13 0x7f89498f25ab in Core::EventLoop::exec() /home/runner/work/serenity/serenity/Userland/Libraries/LibCore/EventLoop.cpp:88:20
    #14 0x557019d6703f in serenity_main(Main::Arguments) /home/runner/work/serenity/serenity/Ladybird/ImageDecoder/main.cpp:23:23
    #15 0x557019d934a8 in main /home/runner/work/serenity/serenity/Userland/Libraries/LibMain/Main.cpp:39:19
    #16 0x7f8948e2a1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #17 0x7f8948e2a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #18 0x557019c7c6e4 in _start (/home/runner/work/serenity/serenity/Meta/Lagom/Build/libexec/ImageDecoder+0x3f6e4) (BuildId: 1d5205a24f26e67958e56b2c6d50455620780dda)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/runner/work/serenity/serenity/Meta/Lagom/../../AK/RefCounted.h:60:37 
nput/table/table-align-center-with-margin.html

I don't think that's something I've seen happen before (on CI or otherwise), but I'm also not sure which of these commits could lead to that happening.

@nico
Copy link
Contributor

nico commented Oct 26, 2025

Can you repro locally and bisect?

@nico
Copy link
Contributor

nico commented Oct 26, 2025

The backing store stuff isn't really needed for us, but I suppose it also doesn't hurt.

The module map is I think for swift interop, but I suppose it also doesn't hurt.

@implicitfield
Copy link
Contributor Author

Can you repro locally and bisect?

Unfortunately, I couldn't repro this locally (all tests pass without any trouble on macOS even with UBSan + ASan). I could try this on Linux later, but that'll be some time away.

The backing store stuff isn't really needed for us, but I suppose it also doesn't hurt.
The module map is I think for swift interop, but I suppose it also doesn't hurt.

Yeah, I was just picking these fairly generously in case any of this helps with future cherry-picks. Though aren't (some of) the backing store changes needed for 386 on our side (based on the discussion in that PR)? At least that part seems to pass CI here now as well.

@implicitfield
Copy link
Contributor Author

implicitfield commented Oct 26, 2025

Cherry-picking be2c484 seems to cause the following (non-fatal) diagnostics to be printed when running the pattern-from-video.html test on Linux:

31: Userland/Libraries/LibGfx/Rect.h:359:62: runtime error: signed integer overflow: -2147483648 + -2147483648 cannot be represented in type 'int'
31: SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Userland/Libraries/LibGfx/Rect.h:359:62 
31: Userland/Libraries/LibGfx/Rect.h:361:63: runtime error: signed integer overflow: -2147483648 + -2147483648 cannot be represented in type 'int'
31: SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Userland/Libraries/LibGfx/Rect.h:361:63

(this also seems to have happened in CI).

I still couldn't replicate the invalid downcast though.

implicitfield and others added 18 commits October 29, 2025 20:33
I forgot to include any when these we added in c972257.
By moving this up to ConnectionBase, we have less custom code for each
templated subclass, and it gets a little easier to edit the code since
you don't have to rebuild as much when making changes.

(cherry picked from commit 6d9c4d8)
(cherry picked from commit 4fe21e6;
amended to add missing includes to various serenity components)
This makes it consistent with other commands.

(cherry picked from commit 877adcc)
This change makes all the pre-commit CI scripts runnable under Bash 3.2,
by replacing “mapfile” invocations in them code that first explicitly
creates an array, and then uses a while loop to populate the array.

Otherwise, without this change, the scripts all fail to run under Bash
3.2 — due to lack of support for “mapfile”.

Fixes LadybirdBrowser/ladybird#283

This also drops bash from the list of homebrew dependencies in the build
instructions — because with this change, homebrew bash (v4) is no longer
needed; things will now work with the Apple-provided bash (v3.2)

(cherry picked from commit 570814a)
When adding tests for `ThreadPool` a handful of deadlocks can be
observed when worker threads wait on `m_work_available`.

The first deadlock is in the destruction of `ThreadPool` where it
is possible for a worker thread to be in the process of acquiring
`m_mutex` when the  broadcast to `m_work_available` in the
destructor happens. This causes the destructor to hang on joining the
thread which is now perpetually waiting on `m_work_available`. This is
solved by repeatedly broadcasting on `m_work_available` until the thread
to join exits.

The second deadlock occurs when the final signal to `m_work_done` is
missed by the wait in `wait_for_all`. At this point all workers are in
the hot loop of attempting to get work from the work queue, however
since there is no work remaining all workers end up waiting on
`m_work_available`. At this point the `wait_for_all` call is also
waiting on `m_work_done`, which will never be signalled again as all
workers are waiting on `m_work_available`.

This requires 2 changes to fix, the first is that workers will signal
`m_done_work` before waiting on `m_work_available`. The second change is
to acquire `m_mutex` before checking the wait conditions as done when
using `wait_while`.

(cherry picked from commit a0fd7cf)
The `ThreadPoolLooper` should increment `m_busy_count` before attempting
to access the global queue. Otherwise, there exists a possible race
condition where `wait_for_all` checks the exit conditions before the
looper increments `m_busy_count` but after it empties the `ThreadPool`
queue.

Next, incrementing / decrementing `m_busy_count` is moved to be the
responsibility of `ThreadPoolLooper`. Otherwise, it is possible that
decrementing `m_busy_count` in the caller of `Looper::next` causes
`m_busy_count` to underflow if the call to `Looper::next` returns
before incrementing `m_busy_count`.

(cherry picked from commit 8d336d2)
This causes issues when loading the file into a clang module, such as
when using Core::MachPort from Objective-C or Swift.

(cherry picked from commit 422f1ed)
NavigatorBeaconMixin doesn't even need to know about Navigator

(cherry picked from commit 35f4b8b)
The PageClient for SVGDecodedImageData needs to actually have the
definition of PageClient available in order to inherit from it. This
fixes importing this header from Objective-C or Swift.

(cherry picked from commit e45b7b0)
On macOS, the first time TabBar::tabSizeHint is called, the count is
reportedly zero, and results in a floating point exception on x64.

(cherry picked from commit cbf1fd3)
Fixes an issue where old prices were not displayed with strike-through
text on the PlayStation store. :^)

(cherry picked from commit 8a6c8a1)
@implicitfield
Copy link
Contributor Author

Removed the backing store / IOSurface changes. Those kind of cause unnecessary trouble, and we won't really be able to cherry-pick most painting-related commits without conflicts anyway.
I think this PR never actually included the module map commit, not sure how I ended up including that in the description.

@implicitfield implicitfield changed the title Cherry-pick 9 pull requests and 9 commits from Ladybird Cherry-pick 6 pull requests and 5 commits from Ladybird Oct 29, 2025
@nico nico merged commit cae7c34 into SerenityOS:master Oct 29, 2025
14 checks passed
@nico
Copy link
Contributor

nico commented Oct 29, 2025

Thanks!

@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 29, 2025
@implicitfield implicitfield deleted the cherry-picks branch October 29, 2025 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants