diff --git a/demos/WebViewAdoptPopupDemo/README.md b/demos/WebViewAdoptPopupDemo/README.md index bba3d38..c35a46f 100644 --- a/demos/WebViewAdoptPopupDemo/README.md +++ b/demos/WebViewAdoptPopupDemo/README.md @@ -32,6 +32,17 @@ and launches the demo. custom UA you set. - **window.open → popup** and the **target=_blank** link also open as tabs. - No native popup window appears at any point. + - **Teardown of an adopted tab** (Canvas 18 Op 10.3). Select an adopted + tab and click **Close tab**: the tab is removed and its + `WebViewComponent` is disposed, which destroys the native engine. The + button does nothing on the opener tab. Repeat several times with each + popup kind above. The app must keep running. + - **window.open → self-closing popup** opens an adopted tab with a + **window.close()** button. Click it (the page asks to close itself), + then close the tab with **Close tab**. Repeat several times. + - Run with `NSZombieEnabled=YES ./run-mac-adopt-popup-demo.sh` so a + message sent to a freed Objective-C object stops the app at once + instead of crashing it later. - **Native window** — the Canvas 15 behaviour: the popup opens in a separate native window (POST preserved, opener-linked). - **Block** — `window.open` returns `null`; the POST form submit is blocked. diff --git a/demos/WebViewAdoptPopupDemo/src/ca/weblite/webview/demos/WebViewAdoptPopupDemo.java b/demos/WebViewAdoptPopupDemo/src/ca/weblite/webview/demos/WebViewAdoptPopupDemo.java index 6f7cbe3..f539990 100644 --- a/demos/WebViewAdoptPopupDemo/src/ca/weblite/webview/demos/WebViewAdoptPopupDemo.java +++ b/demos/WebViewAdoptPopupDemo/src/ca/weblite/webview/demos/WebViewAdoptPopupDemo.java @@ -140,13 +140,14 @@ private static void buildUi() { }); opener.setUrl(openerPage()); - frame.add(buildControls(opener), java.awt.BorderLayout.NORTH); + frame.add(buildControls(opener, tabs), java.awt.BorderLayout.NORTH); addTab(tabs, "opener", opener); frame.add(tabs, java.awt.BorderLayout.CENTER); frame.setVisible(true); } - private static JPanel buildControls(WebViewComponent opener) { + private static JPanel buildControls(WebViewComponent opener, + JTabbedPane tabs) { // Canvas 21 Op 14.4: three rows, not one long FlowLayout line. A row // wider than the frame silently wraps its trailing components out of // view, and in a fixed-height row they become unreachable -- the @@ -206,6 +207,25 @@ private static JPanel buildControls(WebViewComponent opener) { }); bar.add(clearCache); + // Canvas 18 Op 10.3: close the selected ADOPTED tab and dispose it. + // Removing a heavyweight component from its parent does not destroy + // its native peer; dispose() is what reaches cocoa_destroy_engine, so + // this is the step that exercises an adopted child's teardown. The + // opener tab is never closed by this button. + JButton closeTab = new JButton("Close tab"); + closeTab.addActionListener(e -> { + java.awt.Component sel = tabs.getSelectedComponent(); + if (sel == null || sel == opener + || !(sel instanceof WebViewComponent)) { + System.out.println("[demo] Close tab: select an adopted tab"); + return; + } + tabs.remove(sel); + ((WebViewComponent) sel).dispose(); + System.out.println("[demo] closed and disposed an adopted tab"); + }); + bar.add(closeTab); + // Row 2: the User-Agent field takes the slack, its buttons pin right, // so the row's width never depends on the field's preferred size. JPanel uaRow = fieldRow("User-Agent:", uaField, setUa, resetUa); @@ -331,6 +351,15 @@ private static String openerPage() { + "window.open → popup" + "

3. target=_blank link

" + "open example.com" + // Canvas 18 Op 10.3: a popup that closes itself, so the adopted + // child's webViewDidClose: path runs before its tab is closed. + + "

4. window.open → page that closes itself

" + + "');" + + "w.document.close();}\">" + + "window.open → self-closing popup" + "" + ""; diff --git a/spdd/prompt/15-20260802-1000-[Feat]-Browser-Initiated-Popups-Window-Open.md b/spdd/prompt/15-20260802-1000-[Feat]-Browser-Initiated-Popups-Window-Open.md index 5cf8b87..a4a27d0 100644 --- a/spdd/prompt/15-20260802-1000-[Feat]-Browser-Initiated-Popups-Window-Open.md +++ b/spdd/prompt/15-20260802-1000-[Feat]-Browser-Initiated-Popups-Window-Open.md @@ -444,7 +444,9 @@ WebViewPopupCallback ..> PopupDispatcher : delegates to (W/H default to 500×650 — a typical auth-popup size — when unspecified). Create an `NSWindow` (`NSWindowStyleMaskTitled|Closable|Resizable`, contentRect W×H), - set its `contentView` to `child`, `center`, and + **set `releasedWhenClosed` to NO** (see the "The library owns the + popup window" safeguard), set its `contentView` to `child`, + `center`, and `makeKeyAndOrderFront:`. Build a child `Engine` (webview=child, jvm, config=configuration, manager=child's `configuration.userContentController`, `popup_callback` and @@ -459,7 +461,18 @@ WebViewPopupCallback ..> PopupDispatcher : delegates to `Engine`, `[popup_window close]`, fire `onPopupClosed(popupId, …)` async, then tear down the child engine (remove from `g_webview_map`, `setUIDelegate:nil`, delete - global refs, release window + delegate). + global refs, release window + delegate). Before `-close` it clears + the window's delegate, so its own close does not re-enter the + window-close path below. + - **The window's close button tears the popup down too.** The popup's + UI-delegate object also adopts `NSWindowDelegate` and is set as the + popup `NSWindow`'s delegate. Its `windowWillClose:` recovers the + engine through the `"eng"` association; for an engine-owned + native-window popup (not `java_owned`, `popup_window` set) it + schedules the `webViewDidClose:` teardown on the AppKit main queue, + retaining the delegate and the web view across the hop (a window + must not be released inside its own `windowWillClose:`). Java + receives `onPopupClosed` exactly as for `window.close()`. 5. **Linux (WebKitGTK) signal strategy.** Connect three signals on every `WebKitWebView` created by `gtk_create_engine` @@ -857,6 +870,26 @@ File: `test/ca/weblite/webview/PopupDispatcherTest.java` ## S · Safeguards +- **A native popup is torn down once, however it closes (macOS).** A + page's `window.close()` and the window's own close button both reach + the single `webViewDidClose:` teardown, which clears the delegate's + `"eng"` association before anything else; whichever path runs second + recovers nil and does nothing. Without the close-button path, a + user-closed window (created with `releasedWhenClosed` = NO) left the + window, its web view, delegate, configuration, Java global refs and + `Engine` alive, and Java never heard `onPopupClosed`. + +- **The library owns the popup window (macOS).** The engine-owned + popup `NSWindow` is created by `alloc` (+1, held in `popup_window`) + and freed by the single `release` in the `webViewDidClose:` teardown. + It MUST therefore be created with `releasedWhenClosed` set to NO: + left at its default (YES), `-close` releases the window as well, and + the teardown's `release` is one too many. Observed on-device with + `NSZombieEnabled`: a native-window popup that calls `window.close()` + stopped with `-[NSKVONotifying_NSWindow release]: message sent to + deallocated instance` in `objc_autoreleasePoolPop` on the AppKit main + thread. + - **Constructor null-checks.** `WebViewPopupEvent` and `PopupDispatcher` reject null `source` with an NPE named `"source"`. Other Strings coerce null → empty; sizes accept `-1`. diff --git a/spdd/prompt/18-20260806-1200-[Feat]-Popup-Adoption-Into-Webviewcomponent-Tab.md b/spdd/prompt/18-20260806-1200-[Feat]-Popup-Adoption-Into-Webviewcomponent-Tab.md index 866438a..95b71b1 100644 --- a/spdd/prompt/18-20260806-1200-[Feat]-Popup-Adoption-Into-Webviewcomponent-Tab.md +++ b/spdd/prompt/18-20260806-1200-[Feat]-Popup-Adoption-Into-Webviewcomponent-Tab.md @@ -573,6 +573,12 @@ File: `src_c/webview_embed.cpp` (Cocoa) - `NATIVE_WINDOW` (1): exactly today's window path (create child, `NSWindow`, `makeKeyAndOrderFront:`, register child engine, fire `onPopupOpened`). + - **Both non-BLOCK branches**: the child `Engine` **retains** the + `configuration` WebKit passed before storing it as `child_e->config` + (see the "An engine owns every object it releases" safeguard). WebKit + only lends that object for the duration of the delegate call (+0); the + child `WKWebView` keeps its own copy, so the passed object is freed + soon after the call returns unless the engine holds a reference. - `ADOPT` (2): **isolate the child's script world first** — before `initWithFrame:configuration:`, replace the passed `configuration`'s `userContentController` with a **fresh, empty @@ -603,8 +609,12 @@ File: `src_c/webview_embed.cpp` (Cocoa) remove from `g_retained_popups`; return the child engine handle. 4. `cocoa_discard_popup(jlong popupId)`: look up + remove from `g_retained_popups`; `setUIDelegate:nil`; delete inherited global - refs; release the child view; delete the engine (mirror + refs; release the child view; **release the retained + `child_e->config`**; delete the engine (mirror `impl_web_view_did_close` teardown ordering, minus the window). + `impl_web_view_did_close`'s engine-owned path (not `java_owned`) likewise + releases the retained `config` before `delete e`; its `java_owned` path + leaves it for `cocoa_destroy_engine`, which releases it exactly once. 5. JNI bridges `Java_ca_weblite_webview_WebViewNative_webview_1embed_1adopt_1popup` (resolve `parent`'s peer/`NSView` from the AWT `Component` via the @@ -646,6 +656,21 @@ Files: `demos/WebViewAdoptPopupDemo/…` (or a mode on switches handler modes: Adopt (host in a `JFrame`-hosted tab/panel created on `popupAdoptable`), Native window (Canvas 15 allow), Block. Adopt mode logs POST-body round-trip to a console line. +3. **Adopted-child teardown is testable by hand.** The controls carry a + **Close tab** button: when the selected tab is an adopted popup (never + the opener tab, which the button leaves alone), it removes that tab + from the `JTabbedPane` and calls `WebViewComponent.dispose()` on it — + removing a heavyweight component from its parent does not destroy its + native peer, so `dispose()` is the call that reaches + `cocoa_destroy_engine`. The opener page gains a fourth case, + **"window.open → page that closes itself"**: it opens a named popup on + `about:blank` and, through the returned window handle, writes into it a + button that calls `window.close()`, so the adopted child's + `webViewDidClose:` path runs before its tab is closed. The demo README's + checklist lists both steps (repeat each several times) and suggests + running with `NSZombieEnabled=YES` so a remaining over-release faults at + once. These steps verify the "An engine owns every object it releases" + and "An adopted child is freed exactly once" safeguards on macOS. ### 11. README + Tests Files: `README.md`, `test/ca/weblite/webview/PopupDispatcherTest.java` @@ -749,6 +774,21 @@ Files: `README.md`, `test/ca/weblite/webview/PopupDispatcherTest.java` `"eng"` association before freeing, so a `window.close()` that drains after the engine is gone recovers `nil` and no-ops. The two paths are mutually exclusive on `delete`, never racing it. +- **An engine owns every object it releases (no over-release on + destroy).** Every Objective-C object a teardown path releases + (`cocoa_destroy_engine`, `cocoa_discard_popup`, the engine-owned + `impl_web_view_did_close` path) MUST be held at +1 by that `Engine`. + A popup child's `config` is the `WKWebViewConfiguration` WebKit passes + to `webView:createWebViewWithConfiguration:forNavigationAction: + windowFeatures:` — a borrowed (+0) object WebKit frees soon after the + call — so the child engine retains it when storing it, and each + teardown path releases it exactly once. Storing it unretained let + `cocoa_destroy_engine` release a freed object when an adopted tab was + closed: `objc_msgSend` on a garbage isa on the AppKit main thread + (`EXC_BAD_ACCESS` at isa + 0x10), seen in the field after an Okta + dashboard tile opened Jira as an adopted tab. `child_e->manager` stays + a borrowed pointer: it is owned by the child `WKWebView`'s own + configuration and is never released by a teardown path. - **Opener linkage + POST are mandatory (AC1/AC2).** The adopted child MUST be the WebKit-created child from the passed configuration; creating a fresh view or re-navigating from Java is a defect. diff --git a/src_c/webview_embed.cpp b/src_c/webview_embed.cpp index 0bb7dbc..c7bae03 100644 --- a/src_c/webview_embed.cpp +++ b/src_c/webview_embed.cpp @@ -6764,6 +6764,12 @@ static id impl_create_web_view(id self, SEL, id webView, id configuration, win = msg( win, sel("initWithContentRect:styleMask:backing:defer:"), CGRectMake(0, 0, W, H), (unsigned long)15, (unsigned long)2, NO); + // Canvas 15 safeguard "The library owns the popup window": this alloc + // is balanced by the single release in impl_web_view_did_close. Left + // at its default (YES), -close would release the window too, and that + // release would be one too many (NSZombie: -[NSWindow release]: message + // sent to deallocated instance, after a popup's window.close()). + msg(win, sel("setReleasedWhenClosed:"), NO); msg(win, sel("setTitle:"), ns_str("Popup")); msg(win, sel("setContentView:"), child); msg(win, sel("center")); @@ -6776,7 +6782,15 @@ static id impl_create_web_view(id self, SEL, id webView, id configuration, Engine *child_e = new Engine(); child_e->webview = child; child_e->jvm = jvm; - child_e->config = configuration; + // Canvas 18 safeguard "An engine owns every object it releases": WebKit + // only lends `configuration` for this delegate call (+0) and frees it soon + // after; the child WKWebView keeps its own copy. Every teardown path + // releases e->config, so the engine must hold its own +1 here. (Stored + // unretained, cocoa_destroy_engine released a freed object when an adopted + // tab closed -- objc_msgSend on a garbage isa on the AppKit main thread.) + // `manager` stays borrowed: the child WKWebView's own configuration owns + // that controller, and no teardown path releases it. + child_e->config = msg(configuration, sel("retain")); child_e->manager = msg(configuration, sel("userContentController")); child_e->popup_window = win; // nil for ADOPT child_e->popup_id = (jlong)child_e; @@ -6802,6 +6816,12 @@ static id impl_create_web_view(id self, SEL, id webView, id configuration, // the child engine's inherited download_callback (Canvas 23). msg(child, sel("setNavigationDelegate:"), ui); child_e->ui_delegate = ui; + // Canvas 15: the same delegate object hears the NATIVE_WINDOW popup's + // window close (its close button), so a user-closed window is torn down + // like window.close() -- see impl_popup_window_will_close. + if (win) { + msg(win, sel("setDelegate:"), ui); + } { std::lock_guard lk(g_webview_map_mutex); g_webview_map[child] = child_e; @@ -6918,6 +6938,9 @@ static void impl_web_view_did_close(id self, SEL, id webView) { g_retained_popups.erase(pid); } if (e->popup_window) { + // Clear the window's delegate first so this -close does not re-enter + // impl_popup_window_will_close (Canvas 15). + msg(e->popup_window, sel("setDelegate:"), (id)nullptr); msg(e->popup_window, sel("close")); } @@ -6953,9 +6976,39 @@ static void impl_web_view_did_close(id self, SEL, id webView) { // here is a prime candidate for on-device zombie/Instruments validation.) if (e->webview) { msg(e->webview, sel("release")); } e->webview = nullptr; + // Balance the retain taken on the passed configuration in + // impl_create_web_view (Canvas 18 safeguard "An engine owns every object + // it releases"). + if (e->config) { msg(e->config, sel("release")); e->config = nullptr; } delete e; } +// NSWindowDelegate windowWillClose: -- the user closed an engine-owned +// NATIVE_WINDOW popup with its own close button (Canvas 15 safeguard "A native +// popup is torn down once, however it closes"). Runs on AppKit main. The +// teardown is the webViewDidClose: one, scheduled for after this close +// finishes: a window must not be released inside its own windowWillClose:. +// cocoa_run_on_main_async would run it inline here (we are on main), so the +// hop goes through dispatch_async_f. The delegate and the web view are +// retained across the hop. The teardown clears the "eng" association first, +// so if window.close() already tore the popup down, the hop finds nil and +// does nothing. +static void impl_popup_window_will_close(id self, SEL, id /*notification*/) { + Engine *e = (Engine *)objc_getAssociatedObject(self, "eng"); + if (!e || e->java_owned || !e->popup_window || !e->webview) return; + struct Hop { id ui; id webview; }; + Hop *h = new Hop{self, e->webview}; + msg(h->ui, sel("retain")); + msg(h->webview, sel("retain")); + dispatch_async_f(dispatch_get_main_queue(), h, +[](void *p) { + Hop *g = static_cast(p); + impl_web_view_did_close(g->ui, sel("webViewDidClose:"), g->webview); + msg(g->webview, sel("release")); + msg(g->ui, sel("release")); + delete g; + }); +} + // Register (or clear, when cb is null) the Java WebViewPopupCallback for this // engine. Mirrors cocoa_set_dialog_callback. static void cocoa_set_popup_callback(Engine *e, JNIEnv *env, jobject cb) { @@ -7902,6 +7955,15 @@ static Class get_webview_embed_ui_delegate_cls() { c, sel("webViewDidClose:"), (IMP)impl_web_view_did_close, "v@:@"); + // Canvas 15: a NATIVE_WINDOW popup's window close button. + { + Protocol *wd = objc_getProtocol("NSWindowDelegate"); + if (wd) class_addProtocol(c, wd); + } + class_addMethod( + c, + sel("windowWillClose:"), + (IMP)impl_popup_window_will_close, "v@:@"); // Download support -- Canvas 23. The same object serves as the // WKNavigationDelegate and the WKDownloadDelegate: it already // carries the per-engine "eng" associated object every selector @@ -9300,6 +9362,13 @@ static void cocoa_discard_popup(jlong popupId) { msg(e->webview, sel("release")); e->webview = nullptr; } + // Balance the retain taken on the passed configuration in + // impl_create_web_view (Canvas 18 safeguard "An engine owns every + // object it releases"). + if (e->config) { + msg(e->config, sel("release")); + e->config = nullptr; + } delete e; }); }