3 subtests started failing on dom/traversal/TreeWalker-acceptNode-filter-cross-realm.html WPT test after r281520. Looks like the first 3 subtests started failing and the last subtest started passing.
<rdar://problem/87485756>
Likely due to the changes I made to Source/WebCore/bindings/js/JSDOMConvertCallbacks.h so use a different global object.
Created attachment 449061 [details] Patch
Created attachment 449062 [details] Patch
Comment on attachment 449062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449062&action=review > Source/WebCore/bindings/js/JSDOMConvertCallbacks.h:-49 > - return T::create(JSC::asObject(value), &callerGlobalObject(globalObject, vm.topCallFrame)); This doesn't look a right direction to me: we need the incumbent here for backup incumbent scope. Would you mind giving me a bit time to triage the --debug crash in https://bugs.webkit.org/show_bug.cgi?id=232387? Landing it will progress TreeWalker-acceptNode-filter-cross-realm.html to 100%.
(In reply to Alexey Shvayka from comment #5) > Comment on attachment 449062 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449062&action=review > > > Source/WebCore/bindings/js/JSDOMConvertCallbacks.h:-49 > > - return T::create(JSC::asObject(value), &callerGlobalObject(globalObject, vm.topCallFrame)); > > This doesn't look a right direction to me: we need the incumbent here for > backup incumbent scope. > Would you mind giving me a bit time to triage the --debug crash in > https://bugs.webkit.org/show_bug.cgi?id=232387? > Landing it will progress TreeWalker-acceptNode-filter-cross-realm.html to > 100%. if you want to take <rdar://problem/87485756>, go ahead. <rdar://problem/87485756> is currently on me and the safest path for me at the moment (and given this needs to be addressed soon) is to revert my change. I saw your patch but it seems to special case geolocation and that doesn't seem right either.
Our goal is to resolve regressions quickly. With that goal in mind, we usually treat rollback of a recent patch as a reasonable default option. I think it's OK to wait for bug 232387 if it will be resolved quickly. But also, I don't see any downside to reverting r281520, then landing bug 232387, then re-landing r281520. That's the simplest way to honor the goal of avoiding regressions.
(In reply to Geoffrey Garen from comment #7) > Our goal is to resolve regressions quickly. With that goal in mind, we > usually treat rollback of a recent patch as a reasonable default option. > > I think it's OK to wait for bug 232387 if it will be resolved quickly. > > But also, I don't see any downside to reverting r281520, then landing bug > 232387, then re-landing r281520. That's the simplest way to honor the goal > of avoiding regressions. The failing test was authored after r281520 and against bug 232387, so doing that ^ might not guarantee its progression. (In reply to Chris Dumez from comment #6) > if you want to take <rdar://problem/87485756>, go ahead. Thanks, I will make it my topmost priority for today. > I saw your patch but it seems to special case geolocation and that doesn't > seem right either. It does, yes, via [SkipCallbackInvokeCheck] extended attribute, but it's a spec requirement as you noted in r281520.
(In reply to Alexey Shvayka from comment #8) > (In reply to Geoffrey Garen from comment #7) > > Our goal is to resolve regressions quickly. With that goal in mind, we > > usually treat rollback of a recent patch as a reasonable default option. > > > > I think it's OK to wait for bug 232387 if it will be resolved quickly. > > > > But also, I don't see any downside to reverting r281520, then landing bug > > 232387, then re-landing r281520. That's the simplest way to honor the goal > > of avoiding regressions. > > The failing test was authored after r281520 and against bug 232387, so doing > that ^ might not guarantee its progression. > > (In reply to Chris Dumez from comment #6) > > if you want to take <rdar://problem/87485756>, go ahead. > > Thanks, I will make it my topmost priority for today. > > > I saw your patch but it seems to special case geolocation and that doesn't > > seem right either. > > It does, yes, via [SkipCallbackInvokeCheck] extended attribute, but it's a > spec requirement as you noted in r281520. No other browser has implemented r281520 as far as I know and we never shipped it. Therefore, reverting r281520 is safe. Also, clearly r281520 is implemented wrong since it is causing a WPT regressions. It also adds complexity as we can see in your patch attempt, causing you to add a custom IDL attribute. I don't think we should have a [SkipCallbackInvokeCheck] custom attribute and that's why I think your patch is not truly correct. Either the behavior obtained by [SkipCallbackInvokeCheck] is spec-compliant / correct and then it should be applied everywhere (thus not needing a custom IDL attribute) or it is not correct. Special casing Geolocation doesn't make sense IMO.
(In reply to Chris Dumez from comment #9) > (In reply to Alexey Shvayka from comment #8) > > (In reply to Geoffrey Garen from comment #7) > > > Our goal is to resolve regressions quickly. With that goal in mind, we > > > usually treat rollback of a recent patch as a reasonable default option. > > > > > > I think it's OK to wait for bug 232387 if it will be resolved quickly. > > > > > > But also, I don't see any downside to reverting r281520, then landing bug > > > 232387, then re-landing r281520. That's the simplest way to honor the goal > > > of avoiding regressions. > > > > The failing test was authored after r281520 and against bug 232387, so doing > > that ^ might not guarantee its progression. > > > > (In reply to Chris Dumez from comment #6) > > > if you want to take <rdar://problem/87485756>, go ahead. > > > > Thanks, I will make it my topmost priority for today. > > > > > I saw your patch but it seems to special case geolocation and that doesn't > > > seem right either. > > > > It does, yes, via [SkipCallbackInvokeCheck] extended attribute, but it's a > > spec requirement as you noted in r281520. > > No other browser has implemented r281520 as far as I know and we never > shipped it. Therefore, reverting r281520 is safe. Also, clearly r281520 is > implemented wrong since it is causing a WPT regressions. It also adds > complexity as we can see in your patch attempt, causing you to add a custom > IDL attribute. Looks like Firefox landed it in August, probably shipped it already. > I don't think we should have a [SkipCallbackInvokeCheck] custom attribute > and that's why I think your patch is not truly correct. Either the behavior > obtained by [SkipCallbackInvokeCheck] is spec-compliant / correct and then > it should be applied everywhere (thus not needing a custom IDL attribute) or > it is not correct. Special casing Geolocation doesn't make sense IMO. I would love to remove [SkipCallbackInvokeCheck] as well, yet AFAIK browsers invoke some callbacks, like NodeFilter, even if their frame was detached, but not the other ones. There is a spec gap for this.
(In reply to Alexey Shvayka from comment #10) > (In reply to Chris Dumez from comment #9) > > I don't think we should have a [SkipCallbackInvokeCheck] custom attribute > > and that's why I think your patch is not truly correct. Either the behavior > > obtained by [SkipCallbackInvokeCheck] is spec-compliant / correct and then > > it should be applied everywhere (thus not needing a custom IDL attribute) or > > it is not correct. Special casing Geolocation doesn't make sense IMO. > > I would love to remove [SkipCallbackInvokeCheck] as well, yet AFAIK browsers > invoke some callbacks, like NodeFilter, even if their frame was detached, > but not the other ones. There is a spec gap for this. I've tested all callback interfaces and most of the callbacks, and can conclude that Chrome invokes a callback until the lexical global object of the method that registered the callback is alive. Firefox, on the other hand, invokes a callback until its global object is alive. With r281520 and without bug 232387, a callback is invoked until its incumbent global object is alive, which is indeed not correct. If we align with Firefox, which behaviour seems a bit more intuitive to me, we would pass geolocation-API/non-fully-active.https.html, which uses `resolve` / `reject` handlers of the fully active main frame as Geolocation callbacks, without adding [SkipCallbackInvokeCheck]. As for NodeFilter / XPathNSResolver, we can also align them by removing [SkipCallbackInvokeCheck] from them. A nice side effect from that is simplifying return type from CallbackResultType to ExceptionOr. === Considering that revert of r281520 is technically a subset of bug 232387, I suggest we revert r281520 to 1) quickly resolve the regression, 2) keep commit history cleaner, 3) simplify the bug 232387 change.
Created attachment 449190 [details] Patch for landing
Committed r288024 (246050@main): <https://commits.webkit.org/246050@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449190 [details].