Bug 155297 - [CallWith=ScriptExecutionContext] should pass ScriptExecutionContext to the implementation by reference
Summary: [CallWith=ScriptExecutionContext] should pass ScriptExecutionContext to the i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-10 07:33 PST by youenn fablet
Modified: 2016-03-11 01:44 PST (History)
2 users (show)

See Also:


Attachments
Patch (132.28 KB, patch)
2016-03-10 07:37 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (131.83 KB, patch)
2016-03-11 00:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-03-10 07:33:56 PST
[CallWith=ScriptExecutionContext] should pass ScriptExecutionContext to the implementation by reference
Comment 1 youenn fablet 2016-03-10 07:37:46 PST
Created attachment 273566 [details]
Patch
Comment 2 youenn fablet 2016-03-10 08:15:43 PST
(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.
Comment 3 Darin Adler 2016-03-10 08:51:19 PST
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?
Comment 4 youenn fablet 2016-03-10 11:58:00 PST
(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.
Comment 5 youenn fablet 2016-03-11 00:08:21 PST
Created attachment 273693 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-03-11 01:44:43 PST
Comment on attachment 273693 [details]
Patch for landing

Clearing flags on attachment: 273693

Committed r198002: <http://trac.webkit.org/changeset/198002>
Comment 7 WebKit Commit Bot 2016-03-11 01:44:46 PST
All reviewed patches have been landed.  Closing bug.