Add WorkerSettings to WorkerClients to expose certain flag values in WorkerGlobalScope (issue 2171563002 by xlai@chromium.org)

[xl...@chromium.org]

Reviewers: kinuko, Justin Novosad, Stephen Chennney
CL: https://codereview.chromium.org/2171563002/

Message:
kin...@chromium.org: Please review changes in Source/core/workers/

ju...@chromium.org: Please review changes in modules/offscreencanvas2d/ and the
two layout tests

sche...@chromium.org: Please review changes in Source/web/.

Thanks!



Description:
Add WorkerSettings to WorkerClients to expose certain flag values in
WorkerGlobalScope

Currently, Settings() can only be accessed from main thread. So I introduce
WorkerSettings to WorkerClients, such that when WorkerGlobalScope is started,
certain flag values can be passed on to be exposed in worker.

Right now, there is only one flag disableReadingFromCanvas in WorkerSettings (
OffscreenCanvas requires access to the value of flag disableReadingFromCanvas in
order to determine the value of originClean(), regardless of being on main or
worker thread); but it will be convenient for other developers who want to add
more.


BUG=607575

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+212, -4 lines):
A third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
A third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-worker-read-blocked-by-setting.html
M third_party/WebKit/Source/core/core.gypi
A third_party/WebKit/Source/core/workers/WorkerSettings.h
A third_party/WebKit/Source/core/workers/WorkerSettings.cpp
M third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
M third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
M third_party/WebKit/Source/web/DedicatedWorkerGlobalScopeProxyProviderImpl.cpp
M third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
M third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp

[ju...@chromium.org]

What about WebGL? WebGL does not allow cross origin content, but I assume that
it supports this option for blocking toBlob/toDataURL calls. That should work
in a Worker.


https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
File
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
(right):

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html#newcode17
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html:17:
var imageData = context.getImageData(0, 0, 100, 100);
should also test toDataURL and toBlob

https://codereview.chromium.org/2171563002/

[xl...@chromium.org]

But I checked WebGL in offscreencanvas, it doesn't invoke
OffscreenCanvas::originClean(). I think those API functions that need to invoke
that haven't been implemented yet.

I can move the m_disableReadingFromCanvas flag to OffscreenCanvas, instead of
its 2d context class, so that it will be shared by all contexts in
OffscreenCanvas.

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
File
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html
(right):

https://codereview.chromium.org/2171563002/diff/80001/third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html#newcode17
third_party/WebKit/LayoutTests/http/tests/security/offscreen-canvas-read-blocked-by-setting.html:17:
var imageData = context.getImageData(0, 0, 100, 100);

On 2016/07/21 17:27:38, Justin Novosad wrote:
> should also test toDataURL and toBlob

But toDataURL and toBlob have not been implemented in OffscreenCanvas
yet.

https://codereview.chromium.org/2171563002/

[ju...@chromium.org]

On 2016/07/21 17:56:49, xlai (Olivia) wrote:
> But I checked WebGL in offscreencanvas, it doesn't invoke
> OffscreenCanvas::originClean(). I think those API functions that need to
invoke
> that haven't been implemented yet.

That is because the notion of originClean does not make sense for WebGL since it
is not allowed to bring cross-origin content into a WebGL context. That said,
the notion of blocking readback does make sense for webGL. If that flag works
for WebGL, it would not be via origin clean. If it does not currently work for
webGL, please file a bug so that we can fix that.

[ju...@chromium.org]

lgtm for LayoutTests and modules/offscreencanvas2d

https://codereview.chromium.org/2171563002/

[ju...@chromium.org]

On 2016/07/21 18:07:46, Justin Novosad wrote:
> lgtm for LayoutTests and modules/offscreencanvas2d

Bug title should reflect observable change in behaviour (making the
WebKitDisableReadingFromCanvas flag apply to OffscreenCanvases)

https://codereview.chromium.org/2171563002/

[kin...@chromium.org]

(+nhiroki for worker-related changes)


https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp
File third_party/WebKit/Source/core/workers/WorkerSettings.cpp (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp#newcode23
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:23:
WorkerSettings* WorkerSettings::from(ExecutionContext* context)
I'm not sure if we should pass this as a supplement of WorkerClients.
It's handy to pass things from main thread to worker thread, but
Settings isn't a client, and isn't something that's particularly about a
specific module. It needs a little more plumbing but we could probably
just pass it directly via WorkerThreadStartupData?

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp#newcode47
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:47: }
ditto

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.h
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode17
third_party/WebKit/Source/core/workers/WorkerSettings.h:17: class
CORE_EXPORT WorkerSettings : public
GarbageCollectedFinalized<WorkerSettings>, public
Supplement<WorkerClients> {
Does this need to be on-heap? (Just asking)

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode43
third_party/WebKit/Source/core/workers/WorkerSettings.h:43: }
nit: please have empty line before line 43, and make the line 43 as
following for consistency

} // namespace blink

https://codereview.chromium.org/2171563002/

[xl...@chromium.org]

kinuko@: I replied your comments in-line.

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp
File third_party/WebKit/Source/core/workers/WorkerSettings.cpp (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp#newcode23
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:23:
WorkerSettings* WorkerSettings::from(ExecutionContext* context)

On 2016/07/22 09:02:54, kinuko wrote:
> I'm not sure if we should pass this as a supplement of WorkerClients.
It's
> handy to pass things from main thread to worker thread, but Settings
isn't a
> client, and isn't something that's particularly about a specific
module. It
> needs a little more plumbing but we could probably just pass it
directly via
> WorkerThreadStartupData?

I did think of making WorkerSetting to become a member
in WorkerThreadStartupData but was wondering how the workers
team think about the design. The structure is actually simpler.

Also, if WorkerSetting becomes a member in WorkerThreadStartupData.
How do I make sure that the flag values stored in WorkerSetting
remain unchanged when the StartupData is passed from main
to worker thread? Will just doing release() on std::unique_ptr work?

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp#newcode47
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:47: }
On 2016/07/22 09:02:54, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.h
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode17
third_party/WebKit/Source/core/workers/WorkerSettings.h:17: class
CORE_EXPORT WorkerSettings : public
GarbageCollectedFinalized<WorkerSettings>, public
Supplement<WorkerClients> {

On 2016/07/22 09:02:54, kinuko wrote:
> Does this need to be on-heap? (Just asking)

Hmmm, not necessary. But isn't better to be on heap so that we don't
worry about garbage collecting it?

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode43
third_party/WebKit/Source/core/workers/WorkerSettings.h:43: }

On 2016/07/22 09:02:54, kinuko wrote:
> nit: please have empty line before line 43, and make the line 43 as
following
> for consistency
>
> } // namespace blink

Done.

https://codereview.chromium.org/2171563002/

[nhi...@chromium.org]

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp
File third_party/WebKit/Source/core/workers/WorkerSettings.cpp (right):

https://codereview.chromium.org/2171563002/diff/100001/third_party/WebKit/Source/core/workers/WorkerSettings.cpp#newcode23
third_party/WebKit/Source/core/workers/WorkerSettings.cpp:23:
WorkerSettings* WorkerSettings::from(ExecutionContext* context)

On 2016/07/22 21:53:30, xlai (Olivia) wrote:
> On 2016/07/22 09:02:54, kinuko wrote:
> > I'm not sure if we should pass this as a supplement of
WorkerClients. It's
> > handy to pass things from main thread to worker thread, but Settings
isn't a
> > client, and isn't something that's particularly about a specific
module. It
> > needs a little more plumbing but we could probably just pass it
directly via
> > WorkerThreadStartupData?
>
> I did think of making WorkerSetting to become a member
> in WorkerThreadStartupData but was wondering how the workers
> team think about the design. The structure is actually simpler.

I'd prefer to pass it via WorkerThreadStartupData. It'd be more
straightforward.

> Also, if WorkerSetting becomes a member in WorkerThreadStartupData.
> How do I make sure that the flag values stored in WorkerSetting
> remain unchanged when the StartupData is passed from main
> to worker thread? Will just doing release() on std::unique_ptr work?

Could you explain what you're concerned a little more specifically? I'm
not sure who may change the flag values during the period.

In my understanding, WorkerThreadStartupData approach is done as
follows:

1) copy flag values from Settings to WorkerSettings on main thread.
2) create WorkerThreadStartupData and make it own the worker settings on
main thread.
3) post the startup data with its ownership from main thread to worker
thread (see WorkerThread::start())
4) WorkerGlobalScope receives the passed settings and owns it on worker
thread.

At the step 4, the flag values should not be changed from the step 1.

https://codereview.chromium.org/2171563002/

[xl...@chromium.org]

As suggested by kinuko@ and nhiroki@, I changed the WorkerSettings so that it
becomes a member in WorkerThreadStartupData. This WorkerSettings copy flag
values
from Settings on main thread when WorkerThreadStartupData is
created. Then inside createWorkerGlobalScope in WorkerThread::initialize, we
take out the WorkerSetting and put it as a member in WorkerGlobalScope.

On the Offscreen side, as suggested by junov@, I put the
m_disableReadingFromCanvas flag in OffscreenCanvas, so that in the future it can
be shared with WebGL context.

https://codereview.chromium.org/2171563002/

[nhi...@chromium.org]

workers LGTM (you still need owner's stamp)


https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Source/core/workers/WorkerSettings.h
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode21
third_party/WebKit/Source/core/workers/WorkerSettings.h:21: void
copyFlagValuesFromSettings(Settings*);
nit: can you add a blank line between line 21 and 22?

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode26
third_party/WebKit/Source/core/workers/WorkerSettings.h:26: bool
m_disableReadingFromCanvas = false;
How about initializing it on ctor's initializer list and making this
'const'?

https://codereview.chromium.org/2171563002/

[har...@chromium.org]

owner LGTM


https://codereview.chromium.org/2171563002/

[kin...@chromium.org]

Thanks for making the change! The startup data is getting bigger and a little
messier, but I think for this case just adding the field there makes more sense.
LGTM

https://codereview.chromium.org/2171563002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2171563002/250001


https://codereview.chromium.org/2171563002/

[xl...@chromium.org]

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Source/core/workers/WorkerSettings.h
File third_party/WebKit/Source/core/workers/WorkerSettings.h (right):

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode21
third_party/WebKit/Source/core/workers/WorkerSettings.h:21: void
copyFlagValuesFromSettings(Settings*);

On 2016/07/26 02:30:00, nhiroki (slow) wrote:
> nit: can you add a blank line between line 21 and 22?

Done.

https://codereview.chromium.org/2171563002/diff/210001/third_party/WebKit/Source/core/workers/WorkerSettings.h#newcode26
third_party/WebKit/Source/core/workers/WorkerSettings.h:26: bool
m_disableReadingFromCanvas = false;

On 2016/07/26 02:30:00, nhiroki (slow) wrote:
> How about initializing it on ctor's initializer list and making this
'const'?

I can't make it const. This bool variable needs to be assignable so that
copyFlagValuesFromSettings() can work; otherwise there'll be compile
error. Nevertheless, I extract the default values as a separate
function.

https://codereview.chromium.org/2171563002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #13 (id:250001)

https://codereview.chromium.org/2171563002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 13 (id:??) landed as
https://crrev.com/bcb0806adb80a01830af344ce7d423b1cb864ec3
Cr-Commit-Position: refs/heads/master@{#407842}

https://codereview.chromium.org/2171563002/