Must of the complexity in the previous commit had to do with the fact that
about:blank is processed synchronously, meaning that we could process a
scheduled navigation -> page.navigate -> scheduled navigation:
```
let iframe = document.createElement('iframe');
iframe.addEventListner('load', () => {
iframe.src = "about:blank";
});
```
This is an infinite loop which is going to be a problem no mater what, but there
are different degrees of problems this can cause, e.g. looping forever vs use-
after-free or other undefined behavior.
The new approach does 2 passes through scheduled navigations, first processing
"asynchronous" navigation (anything not "about:blank"), then processing
synchronous navigation ("about:blank"). The main advantage is that if the
synchronous navigation causes more synchronous navigation, it won't be
processed until the next tick. PLUS, we can detect about:blank that loads
about:blank and stop it (which might not be to spec, but seems right to do
nonetheless). This 2-pass approach removes the need for a couple of checks and
makes everything else simpler.
It relies on default field values, e.g. for mutex: std.Thread.Mutex = .{}, but
doesn't initialize the structure, just the pointer on the heap resulting in a
crash.
This makes frame sub-navigation "work" for all page navigations (click, form
submit, location.top...) as well as setting the iframe.src.
Fixes at least 2 WPT crashes.
BUT, the implementation still isn't 100% correct, with two known issues:
1. Navigation currently happens in the context where it's called, not the
context of the frame. So if Page1 accesses Frame1 and causes it to navigate,
e.g. f1.contentDocument.querySelector('#link').click(), it's Page1 that will
be navigated, since the JS is being executed in the Page1 context.
This should be relatively easy to fix.
2. There are particularly complicated cases in WPT where a frame is navigated
inside of its own load, creating an endless loop. There's some partial
support for this as-is, but it doesn't work correctly and it currently is
defensive and likely will not continue to navigate. This is particularly true
when sub-navigation is done to about:blank within the frame's on load event.
(Which is probably not a real concern, but an issue for some WPT tests)
Although it shares a lot with the original navigation code, there are many more
edge cases here, possibly due to being developed along side WPT tests. The
source of most of the complexity is the synchronous handling of "about:blank"
in page.navigate, which can result in a scheduled navigation synchronously
causing more scheduled navigation. (Specifically because
`self.documentIsComplete();` is called from page.navigate in that case). It
might be worth seeing if something can be done about that, to simplify this new
code (removing the double queue, removing the flag, simplifying pre-existing
schedule checks ,...)
When we createElement, we assume the element is detached. This is usually true
except for Custom Elements where the constructor can do anything, including
connecting the element. This broken assumption results in cloneNode crashing.
https://github.com/lightpanda-io/browser/pull/1690 narrowed the lifetime of
HandleScopes to once per listener. I think that was just an accident of
refactoring, and not some intentional choice.
The narrower HandleScope lifetime makes it so that when we do run microtask
queue at the end of event dispatching, some locals in the queue may not longer
be valid.
HS1
HS2
queueMicrotask(func)
runMicrotask
In the above flow, `func` is only valid while HS2 is alive, so when we run
the microtask queue in HS1, it is no longer valid.
Playwright, when creating a new CDPSession, sends an
attachToBrowserTarget followed by another attachToTarget to re-attach
itself to the existing target.
see playwright/axtree.js from demo/ repository.
advance() asserts that each byte it steps over is either an ASCII byte
or a UTF-8 sequence leader, never a continuation byte (0x80–0xBF).
consumeName() was calling advance(1) for all non-ASCII bytes
('\x80'...'\xFF'), processing multi-byte sequences one byte at a time.
For a two-byte sequence like é (0xC3 0xA9), the second iteration landed
on the continuation byte 0xA9 and triggered the assertion, crashing the
browser in Debug mode.
Fix: replace advance(1) with consumeChar() for all non-ASCII bytes.
consumeChar() reads the lead byte, derives the sequence length via
utf8ByteSequenceLength, and advances the full code point in one step,
so the position never rests on a continuation byte.
Observed on saintcyrlecole.caliceo.com, whose root element carries an
inline style with custom property names containing French accented
characters (--color-store-bulles-été-fg, etc.). The crash aborted JS
execution before the Angular app could render any dynamic content.
