Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
340363003:
Let ThreadableLoaderClientWrapper's client handle failures better. (Closed)
DescriptionLet ThreadableLoaderClientWrapper's client handle failures better.
If the ThreadableLoaderClientWrapper is notified of a redirect or
CORS access check failure, it forwards that notification to its client.
Do that without first moving the wrapper object to a 'done' state, so as
to allow the client to fully handle the failure -- the worker threadable
loader will for instance signal an error-cancellation failure back at
the wrapper.
R=abarth@chromium.org,tyoshino@chromium.org
BUG=386520
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176945
Patch Set 1 #Patch Set 2 : Include only directly-relevant tests in this CL #Patch Set 3 : Add CSP error-on-redirect test + include other CORS tests #Patch Set 4 : Move all tests to a separate CL; functional change only. #
Total comments: 2
Messages
Total messages: 24 (0 generated)
When you have the time, please take a look.
This CL is difficult to review because you're making a large change to the tests while making a small change to the code. It's hard to see what effect the code change had on the tests through all the noise. Can you split this up into two pieces?
One that refactors the tests and one that makes the behavior change?
On 2014/06/19 16:04:12, abarth wrote: > One that refactors the tests and one that makes the behavior change? The behavior change needs CORS tests to catch the (assert) crashing condition, hence the two cannot be split like that. Do you want me to split up the test changes into smaller batches?
On 2014/06/19 16:22:18, sof wrote: > On 2014/06/19 16:04:12, abarth wrote: > > One that refactors the tests and one that makes the behavior change? > > The behavior change needs CORS tests to catch the (assert) crashing condition, > hence the two cannot be split like that. Do you want me to split up the test > changes into smaller batches? Reduced the test set to only include directly-relevant tests wrt the loader change (which only impacts EventSource.) Will follow up later&separately with the other tests that were reworked here, making them usable from both Document and Worker contexts.
Changed the redirect test to trigger a failure on CSP-redirect, improving coverage slightly. With https://codereview.chromium.org/347043002/ going ahead first, I had to move its CORS test to here. Please take another look?
So as to (hopefully) make progress, I've shifted the CORS tests to a separate CL (https://codereview.chromium.org/345813005/ ) and modified the CL description accordingly. i.e., only the functional change remains - please take another look?
https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/Threa... File Source/core/loader/ThreadableLoaderClientWrapper.h (right): https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/Threa... Source/core/loader/ThreadableLoaderClientWrapper.h:105: m_done = true; Is the client allowed to destroy |this| during the didFailAccessControlCheck callback?
https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/Threa... File Source/core/loader/ThreadableLoaderClientWrapper.h (right): https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/Threa... Source/core/loader/ThreadableLoaderClientWrapper.h:105: m_done = true; On 2014/06/22 14:48:26, abarth wrote: > Is the client allowed to destroy |this| during the didFailAccessControlCheck > callback? I don't think it will be able to accomplish that per the "case 2" lifetime comment next to the WorkerThreadableLoader::MainThreadBridge declaration, i.e., the ref count is upped across the call to the wrapper. No implementation of this method (and didFailRedirectCheck()) will currently try to delete the loader (and indirectly de-ref this wrapper object), but they should be allowed to do so in general. Does that make sense?
On 2014/06/22 at 15:49:14, sigbjornf wrote: > Does that make sense? Yeah. I need to study this class in more detail to be able to review this CL. Other folks should feel free to review it in the meantime.
On 2014/06/22 20:32:27, abarth wrote: > On 2014/06/22 at 15:49:14, sigbjornf wrote: > > Does that make sense? > > Yeah. I need to study this class in more detail to be able to review this CL. > Other folks should feel free to review it in the meantime. Thanks. The change here goes beyond EventSource, but for it, two relevant bugs are: https://bugs.webkit.org/show_bug.cgi?id=61862 https://bugs.webkit.org/show_bug.cgi?id=115104
Cc:ed @horo and @kinuko, who may have looked at lifetime details of this wrapper object as part of http://crbug.com/329786
Can't we fix EventSource than this change? Calling m_loader->cancel() from didFailAccessControlCheck() looks weird and maybe we don't want to add workaround to make it work
On 2014/06/24 12:33:39, tyoshino wrote: > Can't we fix EventSource than this change? Calling m_loader->cancel() from > didFailAccessControlCheck() looks weird and maybe we don't want to add > workaround to make it work Why does it look weird?
On 2014/06/24 12:41:57, sof wrote: > On 2014/06/24 12:33:39, tyoshino wrote: > > Can't we fix EventSource than this change? Calling m_loader->cancel() from > > didFailAccessControlCheck() looks weird and maybe we don't want to add > > workaround to make it work > > Why does it look weird? didFailAccessControlCheck() is notification of completion of DocumentThreadableLoader as failure. The loader is done when didFailAccessControlCheck() is invoked. It doesn't sound so good to me to call cancel() on the finished loader just in order to get didFail() invoked with an error meaning cancellation. It looks EventSource itself can just translate the notification and set m_state to CLOSED without depending on ThreadableLoaderClientWrapper. Sorry if I'm missing any point. Is there any information in the DocumentThreadableLoader the cancel() call is utilizing?
On 2014/06/24 18:00:45, tyoshino wrote: > information in the DocumentThreadableLoader the cancel() call is utilizing? s/DocumentThreadableLoader/WorkerThreadableLoader and ThreadableLoaderClientWrapper/
On 2014/06/24 18:00:45, tyoshino wrote: > On 2014/06/24 12:41:57, sof wrote: > > On 2014/06/24 12:33:39, tyoshino wrote: > > > Can't we fix EventSource than this change? Calling m_loader->cancel() from > > > didFailAccessControlCheck() looks weird and maybe we don't want to add > > > workaround to make it work > > > > Why does it look weird? > > didFailAccessControlCheck() is notification of completion of > DocumentThreadableLoader as failure. The loader is done when > didFailAccessControlCheck() is invoked. It doesn't sound so good to me to call > cancel() on the finished loader just in order to get didFail() invoked with an > error meaning cancellation. It looks EventSource itself can just translate the > notification and set m_state to CLOSED without depending on > ThreadableLoaderClientWrapper. Sorry if I'm missing any point. Is there any > information in the DocumentThreadableLoader the cancel() call is utilizing? I won't recount in prose what WorkerThreadableLoader::cancel() takes care of, but that's where to look. A client explicitly cancelling the loader in response to a failure (generic, CORS, redirect) isn't that unusual, right? Could the protocol-on-failure be changed, and perhaps simplified? Possibly so, but the suggestions above seems too simple. The CL changes makes the existing approach work out in the Worker case, nothing more.
On 2014/06/24 19:55:58, sof wrote: > On 2014/06/24 18:00:45, tyoshino wrote: > > On 2014/06/24 12:41:57, sof wrote: > > > On 2014/06/24 12:33:39, tyoshino wrote: > > > > Can't we fix EventSource than this change? Calling m_loader->cancel() from > > > > didFailAccessControlCheck() looks weird and maybe we don't want to add > > > > workaround to make it work > > > > > > Why does it look weird? > > > > didFailAccessControlCheck() is notification of completion of > > DocumentThreadableLoader as failure. The loader is done when > > didFailAccessControlCheck() is invoked. It doesn't sound so good to me to call > > cancel() on the finished loader just in order to get didFail() invoked with an > > error meaning cancellation. It looks EventSource itself can just translate the > > notification and set m_state to CLOSED without depending on > > ThreadableLoaderClientWrapper. Sorry if I'm missing any point. Is there any > > information in the DocumentThreadableLoader the cancel() call is utilizing? > > I won't recount in prose what WorkerThreadableLoader::cancel() takes care of, > but that's where to look. A client explicitly cancelling the loader in response > to a failure (generic, CORS, redirect) isn't that unusual, right? Sometimes such a design could be chosen for convenience. I agree. But I think we could do better by not to do so here. > > Could the protocol-on-failure be changed, and perhaps simplified? Possibly so, > but the suggestions above seems too simple. The CL changes makes the existing > approach work out in the Worker case, nothing more. I understand that DocumentThreadableLoader also accepts cancel() call even after it has notified completion to m_client. This change makes Worker case consistent to non-Worker case. I'm OK with committing this first. I'll try to refactor it later. Re: lifetime, your reply is fair enough. lgtm
abarth: what do you think - had a chance to look into needed details yet?
On 2014/06/25 at 08:02:47, sigbjornf wrote: > abarth: what do you think - had a chance to look into needed details yet? I don't feel well-informed in this area. Perhaps tyoshino's approval is sufficient?
On 2014/06/25 19:21:59, abarth wrote: > On 2014/06/25 at 08:02:47, sigbjornf wrote: > > abarth: what do you think - had a chance to look into needed details yet? > > I don't feel well-informed in this area. Perhaps tyoshino's approval is > sufficient? certainly it is to me; thanks, i just wanted to hear if you had any additional comments before moving ahead.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/340363003/60001
Message was sent while issue was closed.
Change committed as 176945 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
