Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1253343002:
[XHR] Tidy up files for testing XHR's error for mixed content check failure (Closed)
|
Created:
5 years, 4 months ago by tyoshino (SeeGerritForStatus) Modified:
4 years, 9 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[XHR] Tidy up files for testing XHR's error for mixed content check failure
BUG=389326
R=mkwst,hiroshige
Committed: https://crrev.com/53cd72a69d01081d90806cd7126a7de4258932cf
Cr-Commit-Position: refs/heads/master@{#377839}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Addressed #2 #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Rebase #
Total comments: 2
Patch Set 9 : Reverted message change #Patch Set 10 : Add expected file #
Total comments: 2
Patch Set 11 : Removing a period #Patch Set 12 : Updated the expected file #Patch Set 13 : Revert blank line addition #
Messages
Total messages: 34 (8 generated)
tyoshino@chromium.org changed reviewers: + mkwst@chromium.org
LGTM % test nit. https://codereview.chromium.org/1253343002/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html (right): https://codereview.chromium.org/1253343002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html:5: window.opener.postMessage('done', '*'); Can you add a success handler which postMessages something other than "done", and verify the result in the parent page? As-is, this test will timeout if we break things; I'd rather be explicit about failure.
https://codereview.chromium.org/1253343002/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html (right): https://codereview.chromium.org/1253343002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html:5: window.opener.postMessage('done', '*'); On 2015/07/27 09:09:32, Mike West wrote: > Can you add a success handler which postMessages something other than "done", > and verify the result in the parent page? As-is, this test will timeout if we > break things; I'd rather be explicit about failure. Added!
Saw weird crash on bot. Investigating...
On 2015/07/27 12:07:04, tyoshino wrote:
> Saw weird crash on bot. Investigating...
This seems probably because:
In FetchManager, ThreadableLoader is created as:
m_loader = ThreadableLoader::create(*executionContext(), this, request,
threadableLoaderOptions, resourceLoaderOptions);
However. didFail() is called synchronously in create(), and |m_loader| is
manipulated there, while |m_loader| is being assigned in the line above.
I'll create a fix for this.
On 2015/07/28 07:01:35, hiroshige wrote: > On 2015/07/27 12:07:04, tyoshino wrote: > > Saw weird crash on bot. Investigating... > > This seems probably because: > In FetchManager, ThreadableLoader is created as: > m_loader = ThreadableLoader::create(*executionContext(), this, request, > threadableLoaderOptions, resourceLoaderOptions); > However. didFail() is called synchronously in create(), and |m_loader| is > manipulated there, while |m_loader| is being assigned in the line above. > > I'll create a fix for this. Thanks for investigation. I'm also investigating and started feeling that calling didFail sync is not good. What do you think if we just complete construction and expose the error occurred in the constructor so that the caller can invoke didFail, etc. by themselves? I saw something similar is done for WorkerThreadableLoader.
On 2015/07/28 07:06:24, tyoshino wrote: > On 2015/07/28 07:01:35, hiroshige wrote: > > On 2015/07/27 12:07:04, tyoshino wrote: > > > Saw weird crash on bot. Investigating... > > > > This seems probably because: > > In FetchManager, ThreadableLoader is created as: > > m_loader = ThreadableLoader::create(*executionContext(), this, request, > > threadableLoaderOptions, resourceLoaderOptions); > > However. didFail() is called synchronously in create(), and |m_loader| is > > manipulated there, while |m_loader| is being assigned in the line above. > > > > I'll create a fix for this. > > Thanks for investigation. I'm also investigating and started feeling that > calling didFail sync is not good. What do you think if we just complete > construction and expose the error occurred in the constructor so that the caller > can invoke didFail, etc. by themselves? I saw something similar is done for > WorkerThreadableLoader. Hmm, I'm rethinking. didFail calls in makeCrossOriginAccessRequest() may also call back to the creator sync.
On 2015/07/28 07:01:35, hiroshige wrote: > In FetchManager, ThreadableLoader is created as: > m_loader = ThreadableLoader::create(*executionContext(), this, request, > threadableLoaderOptions, resourceLoaderOptions); > However. didFail() is called synchronously in create(), and |m_loader| is > manipulated there, while |m_loader| is being assigned in the line above. Correction: FetchManager::Loader is destructed in didFail() synchronously, so when ThreadableLoader::create() is returned, |this| is already destructed and thus touching |m_loader| here is use-after-free on FetchManager::Loader. Anyway splitting create() into "create() that returns non-null and doesn't call didFail()" and "start() that can call didFail()" will solve the problem.
The test failures look relevant.
Created a new CL which includes this patch and splits the DocumentThreadableLoader constructor. https://codereview.chromium.org/1264453002/ Hiroshige, Mike, could you please do highlevel design review of it?
Pasting asan log for the record STDERR: ==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070001241c0 at pc 0x7f69c02f967d bp 0x7ffd32c2e170 sp 0x7ffd32c2e168 STDERR: READ of size 8 at 0x6070001241c0 thread T0 (content_shell) STDERR: #0 0x7f69c02f967c in swap<blink::ThreadableLoader *> buildtools/third_party/libc++/trunk/include/type_traits:3535:13 STDERR: #1 0x7f69c02f967c in swap third_party/WebKit/Source/wtf/RefPtr.h:154:0 STDERR: #2 0x7f69c02f967c in operator= third_party/WebKit/Source/wtf/RefPtr.h:134:0 STDERR: #3 0x7f69c02f967c in blink::FetchManager::Loader::performHTTPFetch(bool, bool) third_party/WebKit/Source/modules/fetch/FetchManager.cpp:384:0 STDERR: #4 0x7f69c02f5a07 in blink::FetchManager::Loader::start() third_party/WebKit/Source/modules/fetch/FetchManager.cpp:280:5 STDERR: #5 0x7f69c02fa766 in blink::FetchManager::fetch(blink::ScriptState*, blink::FetchRequestData*) third_party/WebKit/Source/modules/fetch/FetchManager.cpp:435:5 STDERR: #6 0x7f69c0306071 in blink::(anonymous namespace)::GlobalFetchImpl<blink::LocalDOMWindow>::fetch(blink::ScriptState*, blink::RequestOrUSVString const&, blink::Dictionary const&, blink::ExceptionState&) third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:49:16 STDERR: #7 0x7f69c03057a2 in blink::GlobalFetch::fetch(blink::ScriptState*, blink::DOMWindow&, blink::RequestOrUSVString const&, blink::Dictionary const&, blink::ExceptionState&) third_party/WebKit/Source/modules/fetch/GlobalFetch.cpp:126:12
Hiroshige created a bug for documenting ThreadableLoader's behavior: http://crbug.com/514547
On 2015/07/28 07:51:49, hiroshige wrote: > On 2015/07/28 07:01:35, hiroshige wrote: > > In FetchManager, ThreadableLoader is created as: > > m_loader = ThreadableLoader::create(*executionContext(), this, request, > > threadableLoaderOptions, resourceLoaderOptions); > > However. didFail() is called synchronously in create(), and |m_loader| is > > manipulated there, while |m_loader| is being assigned in the line above. > > Correction: > FetchManager::Loader is destructed in didFail() synchronously, so when > ThreadableLoader::create() is returned, |this| is already destructed and thus > touching |m_loader| here is use-after-free on FetchManager::Loader. > > Anyway splitting create() into "create() that returns non-null and doesn't call > didFail()" and "start() that can call didFail()" will solve the problem. Thanks for the explanation. FetchManager::onLoaderFinished() kills the Loader instance and then assigning to m_loader is UaF. RIght?
On 2015/07/28 11:19:42, tyoshino wrote: > Thanks for the explanation. FetchManager::onLoaderFinished() kills the Loader > instance and then assigning to m_loader is UaF. RIght? Right.
Sorry for delay. Quick update: This CL depends on https://codereview.chromium.org/1264453002/.
Description was changed from ========== [XHR] Dispatch error event for mixed content check failure BUG=389326 R=mkwst ========== to ========== Tidy up files for testing XHR's error for mixed content check failure BUG=389326 R=mkwst,hiroshige ==========
On 2015/08/13 at 15:43:52, tyoshino wrote: > Sorry for delay. Quick update: This CL depends on https://codereview.chromium.org/1264453002/. This CL was initially named "[XHR] Dispatch error event for mixed content check failure". But I've included the changes here by mistake (?) in https://codereview.chromium.org/1264453002/ and committed it. I've rebased and now this is just a clean up CL. PTAL.
Description was changed from ========== Tidy up files for testing XHR's error for mixed content check failure BUG=389326 R=mkwst,hiroshige ========== to ========== [XHR] Tidy up files for testing XHR's error for mixed content check failure BUG=389326 R=mkwst,hiroshige ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1253343002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1253343002/140001
https://codereview.chromium.org/1253343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1253343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:841: client->didFail(ResourceError(errorDomainBlinkInternal, 0, requestURL.string(), "Failed to start a loader")); This CL doesn't contain -expected.txt diffs. Does it mean that there is no layout tests that cover this statement and we should add one? (Most of the XHR tests is written by js-test.js and has -expected.txt) nit: What is the reason for this message change? I'm not sure what "a loader" means for JavaScript developers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1253343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1253343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:841: client->didFail(ResourceError(errorDomainBlinkInternal, 0, requestURL.string(), "Failed to start a loader")); On 2016/02/22 at 15:45:46, hiroshige wrote: > This CL doesn't contain -expected.txt diffs. Does it mean that there is no layout tests that cover this statement and we should add one? (Most of the XHR tests is written by js-test.js and has -expected.txt) I was misunderstanding that a test can be without -expected.txt only when there's no CONSOLE message. But it's not. Sorry. We really need a -expected.txt file for this test to verify the printed message. > > nit: What is the reason for this message change? I'm not sure what "a loader" means for JavaScript developers. Good point. I updated this for clarifying internal cause, but this is used for printing a console message. I wasn't aware of that when making this change. I think providing additional and internal details in a console message is not bad (useful when included in a user bug report). Let's refine RawResource::fetchMedia(), etc. in a separate CL to allow DocumentThreadbleLoader to get detailed error info from ResourceFetcher and translate that to a user friendly message. Reverted.
PTAL ps10. Forgot to add the -expected file.
lgtm with a nit. https://codereview.chromium.org/1253343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1253343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:841: client->didFail(ResourceError(errorDomainBlinkInternal, 0, requestURL.string(), "Failed to start loading")); nit: should we remove period in the messages? Some of other messages do contain period (e.g. Line 264), while the others don't (e.g. "Load cancelled"). Probably it's better to make other messages also consistent, or just keep existing messages as-is (including this) if we think this is really nit and feel no need to complicate blame information.
https://codereview.chromium.org/1253343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1253343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:841: client->didFail(ResourceError(errorDomainBlinkInternal, 0, requestURL.string(), "Failed to start loading")); On 2016/02/25 at 16:45:07, hiroshige wrote: > nit: should we remove period in the messages? > Some of other messages do contain period (e.g. Line 264), while the others don't (e.g. "Load cancelled"). > Probably it's better to make other messages also consistent, or just keep existing messages as-is (including this) if we think this is really nit and feel no need to complicate blame information. Woo. Sorry for my bad. I just failed to revert it completely. I'll readd it before committing.
I've also reverted addition of a blank line from DocumentThreadableLoader.cpp
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/1253343002/#ps240001 (title: "Revert blank line addition")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1253343002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1253343002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [XHR] Tidy up files for testing XHR's error for mixed content check failure BUG=389326 R=mkwst,hiroshige ========== to ========== [XHR] Tidy up files for testing XHR's error for mixed content check failure BUG=389326 R=mkwst,hiroshige Committed: https://crrev.com/53cd72a69d01081d90806cd7126a7de4258932cf Cr-Commit-Position: refs/heads/master@{#377839} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/53cd72a69d01081d90806cd7126a7de4258932cf Cr-Commit-Position: refs/heads/master@{#377839} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
