Bug 157006 – JSC should have an option to allow global const redeclarations

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

We will allow for duplicate declarations when the application running is iBooks.

[Saam Barati]

Created attachment 277297 [details]
patch

[Jon Lee]

rdar://problem/25730915

[Geoffrey Garen]

Comment on attachment 277297 [details]
patch

r=me

🙈

[Darin Adler]

Comment on attachment 277297 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277297&action=review

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:280
> +#if PLATFORM(MAC)
> +        namespace ApplicationChecker = MacApplication;
> +#elif PLATFORM(IOS)
> +        namespace ApplicationChecker = IOSApplication;
> +#endif
> +#if PLATFORM(MAC) || PLATFORM(IOS)
> +        if (ApplicationChecker::isIBooks() || !Settings::globalConstRedeclarationShouldThrow())
> +            vm->setGlobalConstRedeclarationShouldThrow(false);
> +#endif

It’s not great to have the policy here. There are three separable concerns:

1) Accurately detecting the application. Handled by the application checkers classes.
2) Accurately implementing the quirks we need. Handled inside the implementation of classes like these.
3) Defining the policy for which apps should trigger which quirks. Longer term, I’d really like to see that in a place separate from (1) and (2) that focuses on the mapping of apps to quirks.

It seems particularly inelegant that the policy is implemented in WebProcessPool::platformInitializeWebProcess for WebKit2, but in JSDOMWindowBase::commonVM for WebKit1.

[Saam Barati]

I need to fix a debug assertion and also make our rule stricter.
Currently, we allow the following:
```
// Program 1:
let foo = 20;
```
```
// Program 2:
const foo = 40;
```

I'm going to prevent this behavior because it doesn't break backwards compat.

[Saam Barati]

Comment on attachment 277297 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277297&action=review

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:280
>> +#endif
> 
> It’s not great to have the policy here. There are three separable concerns:
> 
> 1) Accurately detecting the application. Handled by the application checkers classes.
> 2) Accurately implementing the quirks we need. Handled inside the implementation of classes like these.
> 3) Defining the policy for which apps should trigger which quirks. Longer term, I’d really like to see that in a place separate from (1) and (2) that focuses on the mapping of apps to quirks.
> 
> It seems particularly inelegant that the policy is implemented in WebProcessPool::platformInitializeWebProcess for WebKit2, but in JSDOMWindowBase::commonVM for WebKit1.

I see. So you think the more elegant solution would be to only check a particular Setting in this branch and not the <AppChecker>::isIBooks()?
And then decide on a better place to set the value of that Setting depending on the application? Maybe we can just make Settings::globalConstRedeclarationShouldThrow() check the application as needed?

[Saam Barati]

Created attachment 277406 [details]
patch

Fix debug assertion failures in JSC and improve the strictness of the JSC rule.

(Still haven't addressed Darin's comments)

[Saam Barati]

After discussing this with Geoff, I think *Application::isIBooks
will now work without me doing anything in WK2. I think me doing
this was a vestige of an earlier patch that I did for a similar
compatibility hack. I think I'm going to make the Settings::globalConstRedeclarationShouldThrow a getter that
just checks if the application is iBooks. It won't refer to any fields.
That way, ::commonVM can just ask Settings. And Settings just asks the application
which application it is. commonVM() no longer needs to ask which application is running.

[Saam Barati]

To clarify:
It looks like Chris recently added the capability
of sharing the bundle ID of the UI process. That way
when we check for iBooks in WK1 or WK2, we will get the right answer.

[Saam Barati]

Created attachment 277426 [details]
patch

[Geoffrey Garen]

Comment on attachment 277426 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 277426 [details]
patch

Clearing flags on attachment: 277426

Committed r200121: <http://trac.webkit.org/changeset/200121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 277426 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277426&action=review

> Source/WebCore/page/Settings.h:196
> +    static bool globalConstRedeclarationShouldThrow()
> +    { 
> +#if PLATFORM(MAC)
> +        return !MacApplication::isIBooks();
> +#elif PLATFORM(IOS)
> +        return !IOSApplication::isIBooks();
> +#else
> +        return true;
> +#endif
> +    }

We definitely want to have functions like this that set policy for quirks. I’m not sure, long term, though, if we really want this to be literally in the Settings class. I would love to hear from others about their view of the best place for this sort of thing.

[Darin Adler]

Comment on attachment 277426 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277426&action=review

> Source/WebCore/page/Settings.h:33
> +#include "RuntimeApplicationChecks.h"

Particularly unfortunate to add a new include to a header that’s included so widely. Not sure it’s critical for the globalConstRedeclarationShouldThrow function to be inlined.

Beyond what Darin said, the approach of special-casing all past, present and future versions of a WebKit client is not a good approach to take. The purpose of quirks like this should be to prevent already-deployed software from breaking. New software should not be forced into the special behavior. Instead, new API should be introduced for controlling the behavior, and the only special-casing should be around the default value of the new option. It is often sufficient (and safer) to determine the default value based on the version of WebKit at the time the client linked against the library, rather than on the identity of the client.

I suggest that you rework the fix along those lines.

[Darin Adler]

(In reply to comment #16)
> Beyond what Darin said, the approach of special-casing all past, present and
> future versions of a WebKit client is not a good approach to take. The
> purpose of quirks like this should be to prevent already-deployed software
> from breaking. New software should not be forced into the special behavior.
> Instead, new API should be introduced for controlling the behavior, and the
> only special-casing should be around the default value of the new option. It
> is often sufficient (and safer) to determine the default value based on the
> version of WebKit at the time the client linked against the library, rather
> than on the identity of the client.
> 
> I suggest that you rework the fix along those lines.

Good point. That’s an important refinement for someone to make.

[Geoffrey Garen]

> Beyond what Darin said, the approach of special-casing all past, present and
> future versions of a WebKit client is not a good approach to take.

We think we need to special case book reading forever because the problematic content comes from the book (in this case, published in 2012), rather than the app, and there is no software update mechanism for books.

> Instead, new API should be introduced for controlling the behavior, and the
> only special-casing should be around the default value of the new option.

I do agree -- and I think Saam does, too -- that this setting should be controllable through API. Non-iBooks book readers might have book content that wants this setting too.

I think we should add a WebKit preference similar to -[WebPreferencesPrivate _useSiteSpecificSpoofing]. Perhaps "useEPubQuirks".

[Darin Adler]

(In reply to comment #18)
> > Beyond what Darin said, the approach of special-casing all past, present and
> > future versions of a WebKit client is not a good approach to take.
> 
> We think we need to special case book reading forever because the
> problematic content comes from the book (in this case, published in 2012),
> rather than the app, and there is no software update mechanism for books.

I had the same thought at first.

Then I realized that Dan’s comment is not about when a *quirk* is no longer needed. It’s about special casing *clients* forever.

[Oliver Hunt]

(In reply to comment #19)
> (In reply to comment #18)
> > > Beyond what Darin said, the approach of special-casing all past, present and
> > > future versions of a WebKit client is not a good approach to take.
> > 
> > We think we need to special case book reading forever because the
> > problematic content comes from the book (in this case, published in 2012),
> > rather than the app, and there is no software update mechanism for books.
> 
> I had the same thought at first.
> 
> Then I realized that Dan’s comment is not about when a *quirk* is no longer
> needed. It’s about special casing *clients* forever.

I recall Microsoft implementing behavioral quirks keyed off specific versions of clients. It sounded very much like they had something along the lines of
struct quirks {
bool needsQuirk1 : 1;
bool needsQuirk2 : 1;
etc_t etc; // :D
};

Map<hash, quirks> quirkyDoodads = {
appHash1: {needsQuirk1: true},
appHash2: {needsQuirk2: true},
appHash3: {needsQuirk1: true, needsQuirk2: true}, 
}

or some such.

But maybe that's over engineering here? (MS had to deal with apps depending on truly bizarro behaviors, so would switch out heap allocators, struct sizes, some times even no-op free())

--Oliver

[Saam Barati]

(In reply to comment #15)
> Comment on attachment 277426 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277426&action=review
> 
> > Source/WebCore/page/Settings.h:33
> > +#include "RuntimeApplicationChecks.h"
> 
> Particularly unfortunate to add a new include to a header that’s included so
> widely. Not sure it’s critical for the globalConstRedeclarationShouldThrow
> function to be inlined.

I agree. I'll make a change to move it out of the header.

[Jon Lee]

Filed 157101 to track work for adding the pref.

[Saam Barati]

Created attachment 277538 [details]
follow up to move implementation to cpp file