Regression(r281520) 3 subtests started failing on dom/traversal/TreeWalker-acceptNode-filter-cross-realm.html WPT test

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Radar WebKit Bug Importer]

<rdar://problem/87485756>

[Chris Dumez]

Likely due to the changes I made to Source/WebCore/bindings/js/JSDOMConvertCallbacks.h so use a different global object.

[Chris Dumez]

Created attachment 449061 [details]
Patch

[Chris Dumez]

Created attachment 449062 [details]
Patch

[Alexey Shvayka]

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%.

[Chris Dumez]

(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.

[Geoffrey Garen]

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.

[Alexey Shvayka]

(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.

[Chris Dumez]

(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.

[Alexey Shvayka]

(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.

[Alexey Shvayka]

(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.

[Alexey Shvayka]

Created attachment 449190 [details]
Patch for landing

[EWS]

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].