Bug 267199 – [JSC] Add linked-on-after quirk for canDeclareGlobalFunction()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Richard Northcott]

Created attachment 469323 [details]
Two html files with Javascript functions. Spelling only slightly different.

Since iOS17.2 and now on all WebView-based browsers that I have the following name is defined in the JavaScript function space (where it was not before)

"top()"

eg.

function top()
{

}

It just needs to exist, not even called, and all the JavaScript fails without any feedback.

BraveBrowser reports "Uncaught SyntaxError: Identifier 'top' has already been declared"

This causes webpages and apps using WKWebView in the wild to fail.

It seems that this might be a serious incursion into developer space of window.top or something.
There may be other names that are now broken. I just happened to find this one case.

[Richard Northcott]

The same error occurs for functions named 'location' and 'document'
This would make it seem like it is conflicting with a 'window' property, but other property names do not throw an error.

Here are some that I checked do not cause an error:

right()
left()
bottom()
closed()
getComputedStyle()
getSelection()
history()
innerHeight()
label()
length()
name()
opener()
pageXOffset()
pageYOffset()
parent()
screen()
screenTop()
scrollTo(x, y)

Not an extensive list. There may be more function names that are being rejected)

[Alexey Proskuryakov]

Thank you for the report! Could you please clarify why you consider this to be a bug? The behavior matches Chrome and Firefox on macOS, so all browser engines agree.

[Richard Northcott]

Agreement doesn't make it right ;-)

It is an issue with WebKit itself, not just Safari.

I have been using the function name "top()" in web and app solutions for more than 10 years and with a change in WebKit they all stopped working. -That is why I call it a bug.
If all properties of "window" were no longer allowed to be used as function names, then that would be a 'feature', but I can not see why these names should be blocked out all of a sudden when it will break code in the wild.

[Richard Northcott]

At least the function fails the canDeclareGlobalFunction check, it would be better to skip that function and let the rest be defined.

(Still looking for what is doing this)

[Richard Northcott]

It seems that top is an alias of 'window' itself.

This means that global variables as well as functions are broken. I am sure there are lots of sites using 'top' as a global variable. They not all return the "window" object and therefore broken too.

There must be something like this going on somewhere:
var top = this;

There is something in LocalFrameView* Widget::root(), but I am not sure if this is call on the main page now or something.

Anyway, we need to find where 'window' is being aliased to 'top' and turn it off.

[Alexey Shvayka]

(In reply to Richard Northcott from comment #0)
> Created attachment 469323 [details]
> Two html files with Javascript functions. Spelling only slightly different.
> 
> Since iOS17.2 and now on all WebView-based browsers that I have the
> following name is defined in the JavaScript function space (where it was not
> before)
> 
> "top()"
> 
> eg.
> 
> function top()
> {
> 
> }
> 
> It just needs to exist, not even called, and all the JavaScript fails
> without any feedback.

Yes, it fails to be declared, and aborts execution of the containing script with a SyntaxError.

> This causes webpages and apps using WKWebView in the wild to fail.

Yes, breaking iOS apps was a risk of introducing this SyntaxError, yet apart from this bug report, we haven't recorded any issues so far. As for breaking webpages, both Chrome and Firefox throw an error for `function top() {}` and friends, adhering to the spec (https://tc39.es/ecma262/#sec-globaldeclarationinstantiation).

(In reply to Richard Northcott from comment #1)
> The same error occurs for functions named 'location' and 'document'
> This would make it seem like it is conflicting with a 'window' property, but other property names do not throw an error.

The error occurs for non-configurable non-writable global properties, preventing them from being overridden, which is a good thing: according to Invariants of the Essential Internal Methods (https://tc39.es/ecma262/#sec-invariants-of-the-essential-internal-methods), for [[Get]]:

* If P was previously observed as a non-configurable, non-writable own data property of the target with value V, then [[Get]] must return the SameValue as V.

Introduction of CanDeclareGlobalFunction aligns `window` with other JS objects by enforcing immutability of non-configurable non-writable properties.
Invariants of the Essential Internal Methods were specced in ES6, and the language design committee considers that breaking them for any API designed in the future would prevent developer adoption due to non-sensible behavior.

(In reply to Richard Northcott from comment #3)
> I have been using the function name "top()" in web and app solutions for more than 10 years and with a change in WebKit they all stopped working.
> -That is why I call it a bug.

We would very much like to avoid adding per-app exception for throwing the SyntaxError, so if any way possible, please consider renaming.

[Richard Northcott]

Might I suggest that inside of interpreter.ccp
Interpreter::executeEval
(line 1390)

The section for !canDeclare should be something like this to log but not die:

if (!canDeclare) {
        WTFLogAlways("Error: Cannot declare global function %s", unlinkedFunctionExecutable->name().utf8().data());
        continue;
    }

The same for the variable check.

[Richard Northcott]

It is most likely to be inside of "static void webkit_dom_dom_window_class_init(WebKitDOMDOMWindowClass* requestClass)"


This bit:

	g_object_class_install_property(
        gobjectClass,
        DOM_WINDOW_PROP_TOP,
        g_param_spec_object(
            "top",
            "DOMWindow:top",
            "read-only WebKitDOMDOMWindow* DOMWindow:top",
            WEBKIT_DOM_TYPE_DOM_WINDOW,
            WEBKIT_PARAM_READABLE));
            
Why did this need to be added it would break any Javascript that used a function or variable called "top"?

[Anne van Kesteren]

Richard, I feel like comment 6 explained why we had to make this change. Not throwing would violate the ECMAScript standard. So we cannot really consider suggested changes that would make us non-compliant again.

If this breaks an important app or website that would be good to know, but absent that information I think we will choose to stay compliant.

[Radar WebKit Bug Importer]

<rdar://problem/120965147>

[Richard Northcott]

We have a number of apps that based on the Longman Dictionary brand that we are not able to update due to the license expiring.

Users have paid for about US$8mil worth of these dictionaries -that are now all useless.
There are about 10 titles that are affected. These have been the best selling learner's dictionaries on the AppStore for the past 15 years.

I can send you a promo code, so you can see for yourself.

[Mark Lam]

For reference regarding window.top:
https://developer.mozilla.org/en-US/docs/Web/API/Window/top
https://html.spec.whatwg.org/multipage/nav-history-apis.html#dom-top-dev

[Mark Lam]

Tested this on Safari, Chrome, and Firefox in their respective WebInspector (or equivalent):
```
top
top = function () { return 5; }
top
```

All of them behaves the same: window.top cannot be overridden.

[Mark Lam]

Ran this test on Safari, Chrome, and Firefox (again, via WebInspector):
```
Object.getOwnPropertyDescriptor(window, "top")
```

All of them produced the same result:
```
Object { get: top(), set: undefined, enumerable: true, configurable: false }
```

[Chris Dumez]

Do we know which change caused this? I understand the change in question is good and aligning us with the specification. However, we may want to gate the new behavior on a linked-on-after check so that apps do not get the new behavior until they are rebuilt with the latest iOS SDK.

[Chris Dumez]

https://github.com/WebKit/WebKit/commit/12544d4187162da11dad0451bceece73a83c4837 may explain this behavior change. However, this change was made in iOS 17.0 as far as I can tell. Are you certain this regressed in iOS 17.2?

Also, would the linked-on-after approach I mentioned earlier be a suitable compromise? Note that this means you couldn't rebuild your app with a recent SDK until you can get your code fixed.

[Chris Dumez]

I'll bisect with the provided test case to confirm exactly which change caused this.

[Chris Dumez]

Regression range:
https://commits.webkit.org/compare/267680@main...267651@main

Very likely Alexey's https://github.com/WebKit/WebKit/commit/14f1a47bd7264df4cf795d6ddb88f6e321e8701f I think.

Again, I said regression but it seems like a very intentional change to align us with EcmaScript, in line with other browsers. The bug is on app side.

We may be able to do a linked-on-after check to not break pre-existing apps that can't be updated though.

[Alexey Shvayka]

(In reply to Richard Northcott from comment #11)
> We have a number of apps that based on the Longman Dictionary brand that we
> are not able to update due to the license expiring.
> 
> Users have paid for about US$8mil worth of these dictionaries -that are now
> all useless.
> There are about 10 titles that are affected. These have been the best
> selling learner's dictionaries on the AppStore for the past 15 years.
> 
> I can send you a promo code, so you can see for yourself.

Could you please send a promo code and an app name to my email so I could verify the fix? Thank you.

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/23444

[EWS]

Committed 273773@main (355e9d25900a): <https://commits.webkit.org/273773@main>

Reviewed commits have been landed. Closing PR #23444 and removing active labels.