Bug 155297 – [CallWith=ScriptExecutionContext] should pass ScriptExecutionContext to the implementation by reference

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[youenn fablet]

[CallWith=ScriptExecutionContext] should pass ScriptExecutionContext to the implementation by reference

[youenn fablet]

Created attachment 273566 [details]
Patch

[youenn fablet]

(In reply to comment #1)
> Created attachment 273566 [details]
> Patch

Patch could be completed by additional work on moving to ScriptExecutionContext references wherever possible.
For instance, DOMRequestState constructor should be updated to take a ScriptExecutionContext reference.

[Darin Adler]

Comment on attachment 273566 [details]
Patch

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

Nice change.

Are there some of these where the ScriptExecutionContext argument is not actually needed? Worth thinking about later.

> Source/WebCore/Modules/indexeddb/client/IDBFactoryImpl.cpp:55
> +    ASSERT(is<Document>(context) || context.isWorkerGlobalScope());
> +    if (is<Document>(context)) {
> +        Document& document = downcast<Document>(context);
>          if (!document.frame())
>              return true;
>          if (!document.page())
>              return true;
>      }

In the future seems like this should be expressed as a virtual function on ScriptExecutionContext.

> Source/WebCore/Modules/indexeddb/client/IDBFactoryImpl.cpp:110
> +    ASSERT(context.securityOrigin());
> +    ASSERT(context.topOrigin());

What makes these assertions correct? I know the assertions are not new, but I want to understand why these are guaranteed true and not just "wishful programming".

> Source/WebCore/Modules/indexeddb/client/IDBFactoryImpl.cpp:138
> +    ASSERT(context.securityOrigin());
> +    ASSERT(context.topOrigin());

Ditto.

> Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:165
> +    auto objectStore = IDBObjectStore::create(*scriptExecutionContext(), *info, *this);

What guarantees it can’t be null?

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.idl:31
> +    [SetterCallWith=ScriptExecutionContext] attribute MediaStream? srcObject;

Does this fix a bug? What was the problem before with getting the context from the ActiveDOMObejct function instead of having it be passed?

> Source/WebCore/Modules/notifications/Notification.cpp:226
> +    ASSERT(downcast<Document>(context).page());

This shows us that the argument type should be Document&, not ScriptExecutionContext&. Something we need to eventually work out. It’s really the same problem as the pointer vs. reference thing. Somehow, the caller knows the real type, but the interface does not express that requirement. That gives us the risk later that someone does not understand the requirement at the call site.

> Source/WebCore/Modules/notifications/Notification.cpp:247
> +    ASSERT(downcast<Document>(context).page());

Ditto.

> Source/WebCore/Modules/notifications/NotificationClient.h:47
> +// FIXME: Migrate to ScriptExecutionContext references.

Good idea, but not sure we should add the FIXME.

> Source/WebCore/fileapi/FileReaderSync.h:63
>          return readAsText(scriptExecutionContext, blob, "", ec);

Should be emptyString rather than "" (return later for that I guess).

> Source/WebCore/page/History.h:60
> +    void back(ScriptExecutionContext& context) { back(&context); }
> +    void forward(ScriptExecutionContext& context) { forward(&context); }
> +    void go(ScriptExecutionContext& context, int distance) { go(&context, distance); }
> +
>      void back(ScriptExecutionContext*);
>      void forward(ScriptExecutionContext*);
>      void go(ScriptExecutionContext*, int distance);

Seems unfortunate we need to overload like this. Can we get rid of the pointer versions at some point?

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 273566 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273566&action=review
> 
> Nice change.
> 
> Are there some of these where the ScriptExecutionContext argument is not
> actually needed? Worth thinking about later.
> 
> > Source/WebCore/Modules/indexeddb/client/IDBFactoryImpl.cpp:55
> > +    ASSERT(is<Document>(context) || context.isWorkerGlobalScope());
> > +    if (is<Document>(context)) {
> > +        Document& document = downcast<Document>(context);
> >          if (!document.frame())
> >              return true;
> >          if (!document.page())
> >              return true;
> >      }
> 
> In the future seems like this should be expressed as a virtual function on
> ScriptExecutionContext.

Right.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBFactoryImpl.cpp:110
> > +    ASSERT(context.securityOrigin());
> > +    ASSERT(context.topOrigin());
> 
> What makes these assertions correct? I know the assertions are not new, but
> I want to understand why these are guaranteed true and not just "wishful
> programming".

I do not know the reason.  I'll look at it, time permitting.
That said, these assertions will never crash since context.securityOrigin() is used before them.
I will move the ASSERT() above when landing.

> > Source/WebCore/Modules/indexeddb/client/IDBFactoryImpl.cpp:138
> > +    ASSERT(context.securityOrigin());
> > +    ASSERT(context.topOrigin());
> 
> Ditto.
> 
> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:165
> > +    auto objectStore = IDBObjectStore::create(*scriptExecutionContext(), *info, *this);
> 
> What guarantees it can’t be null?

There is a check above that ensures that.

> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.idl:31
> > +    [SetterCallWith=ScriptExecutionContext] attribute MediaStream? srcObject;
> 
> Does this fix a bug? What was the problem before with getting the context
> from the ActiveDOMObejct function instead of having it be passed?

This is mostly for consistency.
With this change, the setter will throw an exception if the scriptExecutionContext is null in the binding generated code. Without the change, we would just return early without setting anything.

> > Source/WebCore/Modules/notifications/Notification.cpp:226
> > +    ASSERT(downcast<Document>(context).page());
> 
> This shows us that the argument type should be Document&, not
> ScriptExecutionContext&. Something we need to eventually work out. It’s
> really the same problem as the pointer vs. reference thing. Somehow, the
> caller knows the real type, but the interface does not express that
> requirement. That gives us the risk later that someone does not understand
> the requirement at the call site.

The IDL should be updated to set CallWith=Document here.
I will do that as a follow-up patch.

> > Source/WebCore/Modules/notifications/Notification.cpp:247
> > +    ASSERT(downcast<Document>(context).page());
> 
> Ditto.

The IDL should be updated to set CallWith=Document here.
I will do that as a follow-up patch.

> > Source/WebCore/Modules/notifications/NotificationClient.h:47
> > +// FIXME: Migrate to ScriptExecutionContext references.
> 
> Good idea, but not sure we should add the FIXME.

Will remove it.

> > Source/WebCore/fileapi/FileReaderSync.h:63
> >          return readAsText(scriptExecutionContext, blob, "", ec);
> 
> Should be emptyString rather than "" (return later for that I guess).

OK

> > Source/WebCore/page/History.h:60
> > +    void back(ScriptExecutionContext& context) { back(&context); }
> > +    void forward(ScriptExecutionContext& context) { forward(&context); }
> > +    void go(ScriptExecutionContext& context, int distance) { go(&context, distance); }
> > +
> >      void back(ScriptExecutionContext*);
> >      void forward(ScriptExecutionContext*);
> >      void go(ScriptExecutionContext*, int distance);
> 
> Seems unfortunate we need to overload like this. Can we get rid of the
> pointer versions at some point?

This probably require some changes in History IDL.
I will do that as a follow-up patch.

[youenn fablet]

Created attachment 273693 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 273693 [details]
Patch for landing

Clearing flags on attachment: 273693

Committed r198002: <http://trac.webkit.org/changeset/198002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.