Wasn't really needed before frames and multithreading,but it's pretty essential
now to figure out what's going on. Probably needs to be applied more
broadly.
This seems to only happen in error cases, most notably someone changes the
object to return an invalid ownKeys, as we see in WPT
/fetch/api/headers/headers-record.any.html
This is a bit sad, but at least for now, all frames must share the same
page.arena and page.factory (they still get their own v8::Context and
call_arena).
Consider this case (from WPT /css/cssom-view/scrollingElement.html):
```
let nonQuirksFrame = document.createElement("iframe");
nonQuirksFrame.onload = this.step_func_done(function() {
var nonQuirksDoc = nonQuirksFrame.contentDocument;
nonQuirksDoc.removeChild(nonQuirksDoc.documentElement);
});
nonQuirksFrame.src = URL.createObjectURL(new Blob([`<!doctype html>`], { type:
"text/html" }));
document.body.append(nonQuirksFrame);
```
We have the root page (p0) and the frame page (p1). When the frame (p1) is
created, it's [currently] given its own arena/factory and parses the page. Those
nodes are created with p1's factory.
The onload callback executes on p0, so when we call removeChild that's executing
with p0's arena/factory and tries to release memory using that factory - which
is NOT the factory that created them.
A better approach might be that _memory_ operations aren't tied to the current
calling context, but rather the owning document's page. But:
1 - That would mean we have 2 execution contexts: the v8 context where the code
is running, and the memory context that owns the code
2 - Nodes can be disconnected, what page should we use?
3 - Some operations can behave across frames, p0 could adoptNode on p1, so we'd
have to carefully use p1's factory when cleaning up and re-create the node
in p0's factory.
So much hassle.
Using a shared factory/arena solves these problems at the cost of bloat - when
a frame goes away or navigates, we can't free its old memory. At some point, we
should fix that. But I don't have a quick and easy solution, and sharing the
arena/factory is _really_ quick and easy.
Previously, we used a boolean, `_v8_handoff` to detect whether or not an event
was handed off to v8. When it _was_ handed off, then we relied on the Global
finalizer (or context shutdown) to cleanup the instance. When it wasn't handed
off, we could immediately free the instance.
The issue is that, under pressure, v8 might finalize the event _before_ we've
checked the handoff flag. This was the old code:
```zig
const event = try Event.initTrusted(.wrap("DOMContentLoaded"), .{ .bubbles = true }, self);
defer if (!event._v8_handoff) event.deinit(false);
try self._event_manager.dispatch(
self.document.asEventTarget(),
event,
);
```
But what happens if, during the call to dispatch, v8 finalizes the event? The
defer statement will access event after its been freed.
Rather than a boolean, we now track a basic reference count. deinit decreases
the reference count, and only frees the object when it reaches 0. Any handoff
to v8 automatically increases the reference count by 1. The above code becomes
a simpler:
```zig
const event = try Event.initTrusted(.wrap("DOMContentLoaded"), .{ .bubbles = true }, self);
defer event.deinit(false);
try self._event_manager.dispatch(
self.document.asEventTarget(),
event,
);
```
The deinit is un-conditional. The dispatch function itself increases the RC by 1,
and then the v8 handoff increases it to 2. On v8 finalization the RC is
decreased to 1. The defer deinit decreases it to 0, at which point it is freed.
Fixes WPT /css/css-transitions/properties-value-003.html
Should be updated and merged after:
https://github.com/lightpanda-io/browser/pull/1623 else we'll have a double-free.
The ScriptManager used to directly call the "onload" and "onerror" attributes.
The implementation predates EventManager.dispatch support attribute-based
callbacks. But now the EventManager is attribute-aware and correctly times
the attribute dispatch AND details such as cancellation. So this commit moves
the old attribute-only ScriptManager-specific callback to the EventManager.
With one little wrinkle: 'load' listeners added during a script's execution
should NOT receive a 'load' event when the script finishes. This makes no
sense to me. The EventManager now maintains an ignore_list for "load" events
which is reset after each script execution. A comptime flag is passed to
dispatch to indicate whether the ignore list should be checked. This is only
ever set when the ScriptManager dispatches the 'load' event, so there's no
overhead to dispatch for most events.
An amazon product page has 345 resources and not a single DOM "load" listener.
This, I believe, is pretty common (reddit also has no "load" listener). So this
is a simple optimization to skip dispatching the resource "load" event when
there's no listener for it.
The check could be more granular, i.e. checking the specific parents of the
element. But I believe the global no "load" listener is common enough that this
simpler approach is the best.
The worst case is that we dispatch unnecessary "load" events, which is exactly
what the code was doing before.
We often verify the correctness of tests by loading them in an external browser,
but some tests just don't run the same/correctly. For example, we used to hard-
code the http://127.0.0.1:9582/ origin, but that would cause tests to fail if
running from a different origin.
This commit _begins_ the work of improving this. It introduces a
testing.ORIGIN, testing.BASE_URL and testing.HOST which will work correctly in
both our runner and an external browser.
It also introduces `testing.IS_TEST_RUNNER` boolean flag so that tests which
have no chance of working in an external browser (e.g. screen.width) can be
skipped.
The goal is to reduce/remove tests which fail in external browsers so that such
failures aren't quickly written off as "just how it is".
As per spec:
For legacy reasons, load events for resources inside the document (e.g., images)
do not include the Window in the propagation path in HTML implementations.
Define properties directly on the PrototypeTemplate, removing the need for 1
FunctionTemplate per instance-property. Also, properties were previously all
readonly. There is now a readonly flag...thus, properties which we don't care
about, but have a default value, can be defined this way.
Added a 'noop' flag to functions which skips the zig callback and does nothing.
I was initially hoping these would noticeably reduce the size of our snapshot
due to requiring fewer FunctionTemplates. While it shaves a few ~3KB, it's far
less than I was hoping. The issue is that noop function templates still need
to be unique so that `type_a.noopFunc !== type_b.noopFunc` remains true.
Still, as we add more dummy implementation, these little tweaks might add up.
1 - Finalizer callbacks are now give a *Page parameter. Various types no longer
need to maintain a reference to *Page just to finalize
2 - EventManager now handles v8_handoff == false cleanup. This is largely
because of the above change, which would require every:
```
defer if (!event._v8_handoff) event.deinit(false);
```
to be turned into:
```
defer if (!event._v8_handoff) event.deinit(false, page);
```
But the caller might not have a page. Besides this, it makes most uses of Event
simpler. But, in some cases, it could leave a window where the event doesn't
reach the EventManager to be properly managed (though, we have no such cases
as of now).
getImplementation() now returns a cached *DOMImplementation pointer
per Document, matching the getStyleSheets() pattern. This ensures
document.implementation === document.implementation holds true.
Flips dom/nodes/Document-implementation.html (1/2 → 2/2).
- uppercase entire qualified name in tagName (including prefix)
- validate PI data for "?>" and use proper XML Name production with Unicode
- implement willValidate on HTMLInputElement
- throw IndexSizeError DOMException for negative maxLength assignment
flips: Node-nodeName, Document-createProcessingInstruction, button,
maxlength, input-willvalidate (+6 subtests)
Under some conditions, a microtask would be executed for a context that was
already deinit'd, resulting in various use-after-free.
The culprit appears to be WASM compilation being placed in the microtask queue
(by a user-script) and then resolved at some point in the future. We guard the
microtask queue by a context.shutting_down boolean, but v8 doesn't know anything
about this flag. The fact is that, microtasks are tied to an isolate, not a
context.
This commit introduces a number of changes:
1 - It follows 309f254c2c and stores the zig Context inside of an embedder field. This
ensures v8 doesn't consider this when GC'ing, which _could_ extend the
lifetime of the v8::Context beyond what we expect
2 - Most significantly, it introduces per-context microtasks queues. Each
context gets its own queue. This makes cleanup much simpler and reduces the
chance of microtasks outliving the context
3 - pumpMessageLoop is called on context.deinit, this helps to ensure that any
tasks v8 has for our context are processed (e.g. wasm compilation) before
shtudown
4 - The order of context shutdown is important, we notify the isolate of the
context destruction first, then pump the message loop and finally destroy
the context's message loop.
Depends on https://github.com/lightpanda-io/zig-v8-fork/pull/151
- add ChildNode.remove() to DocumentType (flips DocumentType-remove.html)
- return null for MutationRecord.attributeNamespace on non-namespaced
attribute mutations (flips MutationObserver-takeRecords.html)
- stop lowercasing in createElementNS per spec — only createElement
should ASCII-lowercase for HTML namespace (flips
Element/Document-getElementsByTagNameNS.html)
- fix getElementsByTagName to use case-insensitive matching for HTML
namespace elements
