Fix problems with PPB_URLLoader_Impl and PPAPITests.URLLoader.

Patch to fix problems with PPB_URLLoader_Impl and PPAPITests.URLLoader. The cross-origin test doesn't properly check for an error, and the custom-referrer change broke cross-origin requests as a result. Also, there was confusion with some errors being reported as PP_ERROR_FAILED and others as PP_ERROR_NOACCESS. After conversations with WebKit folks, it seems unlikely that a consistent system of error codes can be added, so instead, have PPB_URLLoader_Impl::didFail report PP_ERROR_NOACCESS for unknown error domains (WebKit) and switch on net::kErrorDomain errors from our lower level WebURLLoader.

[jam, 2011-06-10 20:14:24]

i'm really not that familiar with this code, so I'm not a good reviewer

[bbudge, 2011-06-10 22:09:20]

Link to WebKit discussion:
https://bugs.webkit.org/show_bug.cgi?id=61920

[darin (slow to review), 2011-06-11 05:13:12]

http://codereview.chromium.org/7046091/diff/1013/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/1013/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:481: done_status_ =
PP_ERROR_NOACCESS;
Well, the error codes actually are defined by Chromium.  In particular, most
error codes come from the underlying WebURLLoader implementation that lives in
Chromium.  I'm sure we can interpret those here.

Perhaps you sometimes see error codes that do not have domain set to
net::kErrorDomain?  In that case, maybe it makes sense to map those to
PP_ERROR_NOACCESS?

Please also note that for error codes used by WebCore that are not generated by
the network stack, WebCore::FrameLoaderClient methods are called to get access
to them:  e.g., cancelledError, blockedError, cannotShowMIMETypeError, etc.

[bbudge, 2011-06-11 05:21:44]

On 2011/06/11 05:13:12, darin wrote:
>
http://codereview.chromium.org/7046091/diff/1013/webkit/plugins/ppapi/ppb_url...
> File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):
> 
>
http://codereview.chromium.org/7046091/diff/1013/webkit/plugins/ppapi/ppb_url...
> webkit/plugins/ppapi/ppb_url_loader_impl.cc:481: done_status_ =
> PP_ERROR_NOACCESS;
> Well, the error codes actually are defined by Chromium.  In particular, most
> error codes come from the underlying WebURLLoader implementation that lives in
> Chromium.  I'm sure we can interpret those here.
> 
> Perhaps you sometimes see error codes that do not have domain set to
> net::kErrorDomain?  In that case, maybe it makes sense to map those to
> PP_ERROR_NOACCESS?
> 
> Please also note that for error codes used by WebCore that are not generated
by
> the network stack, WebCore::FrameLoaderClient methods are called to get access
> to them:  e.g., cancelledError, blockedError, cannotShowMIMETypeError, etc.

DocumentThreadableLoader creates some ResourceErrors that are all related to
access - would those be FrameLoaderClient::blockedError? I talked with David
Levin and he thought it would be all right to use those. I looked at one
implementation (GTK?) and it seemed to be something else. I'll look at the
errors from net::kErrorDomain.

Another issue that came up is that I can't get the URLLoader test to perform
cross-origin requests successfully. The server always returns an error even if
the rendering code thinks it's OK. Anything I'm missing?

[darin (slow to review), 2011-06-11 05:33:30]

On Fri, Jun 10, 2011 at 10:21 PM, <bbudge@chromium.org> wrote:

> On 2011/06/11 05:13:12, darin wrote:
>
> http://codereview.chromium.**org/7046091/diff/1013/webkit/**
>
plugins/ppapi/ppb_url_loader_**impl.cc<http://codereview.chromium.org/7046091/diff/1013/webkit/plugins/ppapi/ppb_url_loader_impl.cc>
>
>> File webkit/plugins/ppapi/ppb_url_**loader_impl.cc (right):
>>
>
>
> http://codereview.chromium.**org/7046091/diff/1013/webkit/**
>
plugins/ppapi/ppb_url_loader_**impl.cc#newcode481<http://codereview.chromium.org/7046091/diff/1013/webkit/plugins/ppapi/ppb_url_loader_impl.cc#newcode481>
>
>> webkit/plugins/ppapi/ppb_url_**loader_impl.cc:481: done_status_ =
>> PP_ERROR_NOACCESS;
>> Well, the error codes actually are defined by Chromium.  In particular,
>> most
>> error codes come from the underlying WebURLLoader implementation that
>> lives in
>> Chromium.  I'm sure we can interpret those here.
>>
>
>  Perhaps you sometimes see error codes that do not have domain set to
>> net::kErrorDomain?  In that case, maybe it makes sense to map those to
>> PP_ERROR_NOACCESS?
>>
>
>  Please also note that for error codes used by WebCore that are not
>> generated
>>
> by
>
>> the network stack, WebCore::FrameLoaderClient methods are called to get
>> access
>> to them:  e.g., cancelledError, blockedError, cannotShowMIMETypeError,
>> etc.
>>
>
> DocumentThreadableLoader creates some ResourceErrors that are all related
> to
> access - would those be FrameLoaderClient::**blockedError?


Hmm, inspecting the code...

It looks like it just uses errorDomainWebKitInternal with an error code of
0.  I think the
intent is that all such errors are just "access denied" kinds of errors, so
you can probably
just make the Pepper URLLoader code check for the well known
net::kErrorDomain, and
then it can attempt to map some of those error codes to more meaningful
Pepper error
codes.  Otherwise, it can just map to the generic PP_ERROR_NOACCESS.




> I talked with David
> Levin and he thought it would be all right to use those. I looked at one
> implementation (GTK?) and it seemed to be something else. I'll look at the
> errors from net::kErrorDomain.
>
> Another issue that came up is that I can't get the URLLoader test to
> perform
> cross-origin requests successfully. The server always returns an error even
> if
> the rendering code thinks it's OK. Anything I'm missing?


I'm not sure what's up with that.  It would seem that we need to have two
servers
running, or at least two mocks pointing to the same testserver.

-Darin


>
>
>
http://codereview.chromium.**org/7046091/<http://codereview.chromium.org/7046...
>

[bbudge, 2011-06-13 18:33:08]

Thanks for the suggestion about net::kErrorDomain.

> > Another issue that came up is that I can't get the URLLoader test to
> > perform
> > cross-origin requests successfully. The server always returns an error even
> > if
> > the rendering code thinks it's OK. Anything I'm missing?
> 
> 
> I'm not sure what's up with that.  It would seem that we need to have two
> servers
> running, or at least two mocks pointing to the same testserver.
> 
> -Darin
> 
> 
I can start another TestServer, but I'm not sure how to pass its URL to the
tests, which run as a plugin. The test page URL is passed to the plugin via the
WebFrame. I'm looking for a way to get the other URL (also localhost, but
another port) to the plugin. Ideas?

[darin (slow to review), 2011-06-13 21:38:44]

On Mon, Jun 13, 2011 at 11:33 AM, <bbudge@chromium.org> wrote:

> Thanks for the suggestion about net::kErrorDomain.
>
>
>  > Another issue that came up is that I can't get the URLLoader test to
>> > perform
>> > cross-origin requests successfully. The server always returns an error
>> even
>> > if
>> > the rendering code thinks it's OK. Anything I'm missing?
>>
>
>
>  I'm not sure what's up with that.  It would seem that we need to have two
>> servers
>> running, or at least two mocks pointing to the same testserver.
>>
>
>  -Darin
>>
>
>
> I can start another TestServer, but I'm not sure how to pass its URL to the
> tests, which run as a plugin. The test page URL is passed to the plugin via
> the
> WebFrame. I'm looking for a way to get the other URL (also localhost, but
> another port) to the plugin. Ideas?


Yeah, that sounds tricky.  We could talk about this in person perhaps.

-Darin


>
>
>
http://codereview.chromium.**org/7046091/<http://codereview.chromium.org/7046...
>

[polina, 2011-06-14 07:53:28]

http://codereview.chromium.org/7046091/diff/9001/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/9001/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:252: return PP_ERROR_BADARGUMENT;
have you considered changing this to PP_ERROR_NOACCESS like we talked?

[bbudge, 2011-06-15 20:26:19]

http://codereview.chromium.org/7046091/diff/9001/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/9001/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:252: return PP_ERROR_BADARGUMENT;
On 2011/06/14 07:53:28, polina wrote:
> have you considered changing this to PP_ERROR_NOACCESS like we talked?
This is a tough call. If you look at the implementation of
RequiresUniversalAccess, all it does is check for a custom referrer. So this
method name is slightly misleading. Is there a reason you want this to be
PP_ERROR_NOACCESS?

[polina, 2011-06-15 22:46:34]

http://codereview.chromium.org/7046091/diff/9001/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/9001/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:252: return PP_ERROR_BADARGUMENT;
On 2011/06/15 20:26:20, bbudge1 wrote:
> On 2011/06/14 07:53:28, polina wrote:
> > have you considered changing this to PP_ERROR_NOACCESS like we talked?
> This is a tough call. If you look at the implementation of
> RequiresUniversalAccess, all it does is check for a custom referrer. So this
> method name is slightly misleading. Is there a reason you want this to be
> PP_ERROR_NOACCESS?

Seemed to match the error better based on the method names. But I'm happy to
rely on your knowledge of the internals if you think it is not a good idea.

[bbudge, 2011-06-15 22:52:36]

It turns out to be easier to get a cross-origin URL for the test document rather
than one of the data docs. That requires either making a request and getting the
resolved URL, or knowledge of how ppapi_uitests sets up the server.

http://codereview.chromium.org/7046091/diff/21001/ppapi/tests/test_case.html....
File ppapi/tests/test_case.html.mock-http-headers (right):

http://codereview.chromium.org/7046091/diff/21001/ppapi/tests/test_case.html....
ppapi/tests/test_case.html.mock-http-headers:3: Access-Control-Allow-Origin: *
It turned out to be easier to use the test document for the cross-origin
request. Otherwise, I'd have to do a request to get the resolved URL.

[bbudge, 2011-06-15 22:56:56]

http://codereview.chromium.org/7046091/diff/21001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/21001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:252: return PP_ERROR_BADARGUMENT;
I'm going to change this to PP_ERROR_NOACCESS per Polina's request.

[darin (slow to review), 2011-06-15 23:00:20]

http://codereview.chromium.org/7046091/diff/21001/ppapi/tests/test_url_loader.cc
File ppapi/tests/test_url_loader.cc (right):

http://codereview.chromium.org/7046091/diff/21001/ppapi/tests/test_url_loader...
ppapi/tests/test_url_loader.cc:284: std::string cross_origin_url =
document_url.replace(
you might want to assert that thing you are replacing is not "localhost"... that
way you can be sure that the test is meaningful.

http://codereview.chromium.org/7046091/diff/21001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/21001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:484: // For our 'net' error domain,
report all non-access errors as
nit: these comments just summarize what the code does.  maybe they aren't
needed?

I think the code might be plenty readable without so many comments:

  if (...conditions...) {
    // TODO(bbudge): Extend pp_errors.h to cover interesting network errors.
    done_status_ = PP_ERROR_FAILED;
  } else {
    // Assume we encountered some kind of access error.
    done_status_ = PP_ERROR_NOACCESS;
  }

It would probably be very nice to extend pp_errors.h to have interesting network
related error codes like "connection failed", "host not found", etc.  I'd expect
to replace PP_ERROR_FAILED with some kind of mapping function that understands
net_errors.h.

[bbudge, 2011-06-15 23:50:21]

http://codereview.chromium.org/7046091/diff/21001/ppapi/tests/test_url_loader.cc
File ppapi/tests/test_url_loader.cc (right):

http://codereview.chromium.org/7046091/diff/21001/ppapi/tests/test_url_loader...
ppapi/tests/test_url_loader.cc:284: std::string cross_origin_url =
document_url.replace(
On 2011/06/15 23:00:20, darin wrote:
> you might want to assert that thing you are replacing is not "localhost"...
that
> way you can be sure that the test is meaningful.

Done.

http://codereview.chromium.org/7046091/diff/21001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/7046091/diff/21001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:484: // For our 'net' error domain,
report all non-access errors as
On 2011/06/15 23:00:20, darin wrote:
> nit: these comments just summarize what the code does.  maybe they aren't
> needed?
> 
> I think the code might be plenty readable without so many comments:
> 
>   if (...conditions...) {
>     // TODO(bbudge): Extend pp_errors.h to cover interesting network errors.
>     done_status_ = PP_ERROR_FAILED;
>   } else {
>     // Assume we encountered some kind of access error.
>     done_status_ = PP_ERROR_NOACCESS;
>   }
> 
> It would probably be very nice to extend pp_errors.h to have interesting
network
> related error codes like "connection failed", "host not found", etc.  I'd
expect
> to replace PP_ERROR_FAILED with some kind of mapping function that understands
> net_errors.h.

Done.

[darin (slow to review), 2011-06-16 05:15:29]

LGTM

[commit-bot: I haz the power, 2011-06-16 15:04:51]

Try job failure for 7046091-32001 on linux for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[bbudge, 2011-06-16 19:46:07]

On 2011/06/16 15:04:51, I haz the power (commit-bot) wrote:
> Try job failure for 7046091-32001 on linux for step "compile" (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

Replaced -1 with std::string::npos to fix compile on Linux.

[darin (slow to review), 2011-06-16 19:49:21]

OK, LGTM

On Thu, Jun 16, 2011 at 12:46 PM, <bbudge@chromium.org> wrote:

> On 2011/06/16 15:04:51, I haz the power (commit-bot) wrote:
>
>> Try job failure for 7046091-32001 on linux for step "compile" (clobber
>> build).
>> It's a second try, previously, step "compile" failed.
>>
>
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=linux&**number=32391<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&number=32391>
>
> Replaced -1 with std::string::npos to fix compile on Linux.
>
>
>
http://codereview.chromium.**org/7046091/<http://codereview.chromium.org/7046...
>

[commit-bot: I haz the power, 2011-06-16 21:48:38]

Change committed as 89405