Canonicalize uris in C++ instead of Dart for the standalone embedder.

Canonicalize uris in C++ instead of Dart for the standalone embedder.

Adds Dart_DefaultCanonicalizeUrl() to the dart embedding api.

Motivation:

As we try to get source reloading working for the standalone embedder, things get simpler if an isolate doesn't run Dart code while it is loading Dart code.  We intend to solve this by moving the embedder tag handler calls to the service isolate.  But making a blocking rpc into the service isolate whenever a url needs to be canonicalized during parsing seems like it would slow things down and make things complicated.  By moving canonicalization into C++, we avoid this.

R=ahe@google.com, fschneider@google.com, johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/a7d46eb5a687f5b12571966c1df17660877a2ff9

[turnidge, 2016-05-24 19:55:09]

turnidge@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-05-24 23:53:46]

LGTM (with a some nits).

I just have some questions around the API shape as I would like to get rid of
_resolveScriptUri from builtin.dart.

https://codereview.chromium.org/2011543002/diff/80001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/bin/builtin.dar...
runtime/bin/builtin.dart:328: Uri _resolveScriptUri(String scriptName) {
We need to get rid of this function too. If new API allowed providing a base
directly, we could just call it with the working directory and the script name.

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:2702: DART_EXPORT Dart_Handle
Dart_DefaultCanonicalizeUrl(Dart_Handle library,
If we had another variant of this API call that took the base url explicitly we
could use it to replace the other _resolve? Dart functions which just need to
resolve a path relative to some base uri.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:92: char* buffer = zone->Alloc<char>(len * 3 + 1);  // +1 for
'\0'
nit: (len * 3) + 1

maybe a comment about the * 3 being the worst case.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:107: OS::SNPrint(buffer + buffer_pos, 4, "%%%02X",
escaped_value);
Would copying the the three characters from the original string be faster here?

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:471: if (base.scheme != NULL && strcmp(base.scheme, "dart") ==
0) {
nits about parenthesis:

(base.scheme != NULL) && (strcmp(base.scheme, "dart") == 0)

[ahe, 2016-05-25 12:44:51]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2016-05-25 12:44:52]

I'm confused about NormalizeEscapes.

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:2696: *                resolved.
Consider documenting what the Dart type of library and url are.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5168: const char* resolved_uri;
I don't understand how memory is managed for this.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:98: int escaped_value = GetEscapedValue(str, pos, len);
I don't understand this code. As far as I can tell, the string "41" would be
normalized to "A", and "fa.com" would become "%FA.com".

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.h
File runtime/vm/uri.h (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.h#newcode13
runtime/vm/uri.h:13: class Zone;
What's this for?

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri_test.cc
File runtime/vm/uri_test.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri_test.cc#...
runtime/vm/uri_test.cc:543: }
I can't tell if this is a port of tests/corelib/uri_test.dart. If not, I suggest
that you attempt to port the relevant parts of it.

[Florian Schneider, 2016-05-26 14:09:44]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2016-05-26 14:09:45]

dbc: Please add a 1-2 sentence motivation to the CL description.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5168: const char* resolved_uri;
On 2016/05/25 12:44:52, ahe wrote:
> I don't understand how memory is managed for this.

The string is in zone-allocated memory.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:14: char c = str[i];
Why not use libc tolower?

[turnidge, 2016-05-27 21:40:06]

This is not a response to all comments yet, but should allow us to make faster
progress.  Peter please take another look.

Todd

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:98: int escaped_value = GetEscapedValue(str, pos, len);
On 2016/05/25 12:44:52, ahe wrote:
> I don't understand this code. As far as I can tell, the string "41" would be
> normalized to "A", and "fa.com" would become "%FA.com".

I think you probably need to look at GetEscapedValue again.  GetEscapedValue
checks for a '%' character (see line 74).  It returns the value of the escape
sequence if it is valid and a negative number otherwise.

At the risk of being too verbose:

"%41" is unreserved and turns into "A"
"%20" is not unreserved and will be left as "%20"
"%1b" is not unreserved and will be uppercased as "%1B"

Also:

"A" is unreserved and will be left alone.
";' is a delimiter and will be left alone.
" " is neither unreserved nor a delimiter and will become "%20"

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:107: OS::SNPrint(buffer + buffer_pos, 4, "%%%02X",
escaped_value);
On 2016/05/24 23:53:46, Cutch wrote:
> Would copying the the three characters from the original string be faster
here?

By reprinting the escaped value we normalize it to upper case.  Spec suggests
this.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:471: if (base.scheme != NULL && strcmp(base.scheme, "dart") ==
0) {
On 2016/05/24 23:53:46, Cutch wrote:
> nits about parenthesis:
> 
> (base.scheme != NULL) && (strcmp(base.scheme, "dart") == 0)

Done.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.h
File runtime/vm/uri.h (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.h#newcode13
runtime/vm/uri.h:13: class Zone;
On 2016/05/25 12:44:52, ahe wrote:
> What's this for?

Nothing!  Removed.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri_test.cc
File runtime/vm/uri_test.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri_test.cc#...
runtime/vm/uri_test.cc:543: }
On 2016/05/25 12:44:52, ahe wrote:
> I can't tell if this is a port of tests/corelib/uri_test.dart. If not, I
suggest
> that you attempt to port the relevant parts of it.

Well if it wasn't clear, it wasn't a port.

I have added a number of tests from uri_test.dart to this file.  In particular,
I have added a lot of tests that have to do with uri resolution.

While doing this, I noticed that the dart Uri class supports resolved a relative
uri against another relative uri which is not in the spec but seems quite
reasonable.  I have implemented this in my code as well.  I added some
additional tests to uri_test.dart which were helpful for me in nailing down this
behavior which is outside the spec.

[turnidge, 2016-05-31 18:22:57]

Description was changed from

==========
Canonicalize uris in C++ instead of Dart for the standalone embedder.

Adds Dart_DefaultCanonicalizeUrl() to the dart embedding api.
==========

to

==========
Canonicalize uris in C++ instead of Dart for the standalone embedder.

Adds Dart_DefaultCanonicalizeUrl() to the dart embedding api.

Motivation:

As we try to get source reloading working for the standalone embedder, things
get simpler if an isolate doesn't run Dart code while it is loading Dart code. 
We intend to solve this by moving the embedder tag handler calls to the service
isolate.  But making a blocking rpc into the service isolate whenever a url
needs to be canonicalized during parsing seems like it would slow things down
and make things complicated.  By moving canonicalization into C++, we avoid
this.
==========

[turnidge, 2016-05-31 18:25:27]

PTAL.  I believe I have responded to all comments.  Any more comments Peter or
Florian?  John is on vacation and gave me his LGTM before he left.

Florian: I added this to the CL description:

As we try to get source reloading working for the standalone embedder, things
get simpler if an isolate doesn't run Dart code while it is loading Dart code. 
We intend to solve this by moving the embedder tag handler calls to the service
isolate.  But making a blocking rpc into the service isolate whenever a url
needs to be canonicalized during parsing seems like it would slow things down
and make things complicated.  By moving canonicalization into C++, we avoid
this.

https://codereview.chromium.org/2011543002/diff/80001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/bin/builtin.dar...
runtime/bin/builtin.dart:328: Uri _resolveScriptUri(String scriptName) {
On 2016/05/24 23:53:46, Cutch wrote:
> We need to get rid of this function too. If new API allowed providing a base
> directly, we could just call it with the working directory and the script
name.

I would rather hold off on exposing the api in that form for a bit, until we
merge your upcoming changes with my work.

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:2696: *                resolved.
On 2016/05/25 12:44:52, ahe wrote:
> Consider documenting what the Dart type of library and url are.

Documented that url is a string handle.  The documentation of library is
consistent with the rest of this file, for better or worse.

https://codereview.chromium.org/2011543002/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:2702: DART_EXPORT Dart_Handle
Dart_DefaultCanonicalizeUrl(Dart_Handle library,
On 2016/05/24 23:53:46, Cutch wrote:
> If we had another variant of this API call that took the base url explicitly
we
> could use it to replace the other _resolve? Dart functions which just need to
> resolve a path relative to some base uri.

Responded on other comment.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:14: char c = str[i];
On 2016/05/26 14:09:45, Florian Schneider wrote:
> Why not use libc tolower?

I only wanted ASCII characters to be modified -- I was worried that tolower
would use the current locale.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:92: char* buffer = zone->Alloc<char>(len * 3 + 1);  // +1 for
'\0'
On 2016/05/24 23:53:46, Cutch wrote:
> nit: (len * 3) + 1
> 
> maybe a comment about the * 3 being the worst case.

Done.

[ahe, 2016-05-31 21:44:42]

lgtm

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:82: // Invalid escape sequence.  Ignore it.
What does the RFC say about this situation?

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:98: int escaped_value = GetEscapedValue(str, pos, len);
On 2016/05/27 21:40:05, turnidge wrote:
> I think you probably need to look at GetEscapedValue again.  GetEscapedValue
> checks for a '%' character (see line 74). 

This is embarrassing, I spent a lot of time looking at that, and I had somehow
convinced myself that line 74 said "if (str[pos] == '%') {". Thank you for
helping me understand this.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:395: // degenerate case, but it occurs when we resolve
relative urls
I wouldn't call this a degenerate case. It's quite common to not provide a
scheme.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:471: if (base.scheme != NULL && strcmp(base.scheme, "dart") ==
0) {
This is odd. Why is there a special case for the dart scheme?

https://codereview.chromium.org/2011543002/diff/120001/tests/corelib/uri_test...
File tests/corelib/uri_test.dart (right):

https://codereview.chromium.org/2011543002/diff/120001/tests/corelib/uri_test...
tests/corelib/uri_test.dart:139: testResolve("../../../d", "../../../d");
Nice that you updated this!

[Florian Schneider, 2016-06-01 07:13:33]

Thanks for adding a short description!

Here are two more comments.

Lgtm otherwise!

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:14: char c = str[i];
On 2016/05/31 18:25:27, turnidge wrote:
> On 2016/05/26 14:09:45, Florian Schneider wrote:
> > Why not use libc tolower?
> 
> I only wanted ASCII characters to be modified -- I was worried that tolower
> would use the current locale.

Ok, it may be worth adding a short comment.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:48: return ((value >- '0' && value <= '9') ||
-'0' does not seem right.

s/>-/>=/

[turnidge, 2016-06-01 20:00:40]

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
File runtime/vm/uri.cc (right):

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:14: char c = str[i];
On 2016/06/01 07:13:33, Florian Schneider wrote:
> On 2016/05/31 18:25:27, turnidge wrote:
> > On 2016/05/26 14:09:45, Florian Schneider wrote:
> > > Why not use libc tolower?
> > 
> > I only wanted ASCII characters to be modified -- I was worried that tolower
> > would use the current locale.
> 
> Ok, it may be worth adding a short comment.

Done.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:48: return ((value >- '0' && value <= '9') ||
On 2016/06/01 07:13:33, Florian Schneider wrote:
> -'0' does not seem right.
> 
> s/>-/>=/

Nice catch.  Thanks.  Fixed.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:82: // Invalid escape sequence.  Ignore it.
On 2016/05/31 21:44:42, ahe wrote:
> What does the RFC say about this situation?

I couldn't exactly figure it out, but my best guess is that a parser should
reject the uri as invalid.  There are many invalid uris that my C++
implementation does not reject, because I don't do full validation on all
subcomponents.  So I think it is ok if I pass these invalid uris through.

There is a unit test that checks for an invalid escape sequence in a data uri. 
That test still passes.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:395: // degenerate case, but it occurs when we resolve
relative urls
On 2016/05/31 21:44:42, ahe wrote:
> I wouldn't call this a degenerate case. It's quite common to not provide a
> scheme.

Corrected the comment.

https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
runtime/vm/uri.cc:471: if (base.scheme != NULL && strcmp(base.scheme, "dart") ==
0) {
On 2016/05/31 21:44:42, ahe wrote:
> This is odd. Why is there a special case for the dart scheme?

Discussed offline.  Sometimes we resolve a relative uri inside of a dart:blah
library and it is handled specially by the system.

[turnidge, 2016-06-01 20:01:57]

Also, I chose to remove the code which resolves against a relative (no-scheme,
no-authority) base url.  We don't need it in C++ and there were some corner
cases that I was getting wrong.  For now that case is rejected and I can add it
later if needed.

On 2016/06/01 20:00:40, turnidge wrote:
> https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc
> File runtime/vm/uri.cc (right):
> 
>
https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
> runtime/vm/uri.cc:14: char c = str[i];
> On 2016/06/01 07:13:33, Florian Schneider wrote:
> > On 2016/05/31 18:25:27, turnidge wrote:
> > > On 2016/05/26 14:09:45, Florian Schneider wrote:
> > > > Why not use libc tolower?
> > > 
> > > I only wanted ASCII characters to be modified -- I was worried that
tolower
> > > would use the current locale.
> > 
> > Ok, it may be worth adding a short comment.
> 
> Done.
> 
>
https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
> runtime/vm/uri.cc:48: return ((value >- '0' && value <= '9') ||
> On 2016/06/01 07:13:33, Florian Schneider wrote:
> > -'0' does not seem right.
> > 
> > s/>-/>=/
> 
> Nice catch.  Thanks.  Fixed.
> 
>
https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
> runtime/vm/uri.cc:82: // Invalid escape sequence.  Ignore it.
> On 2016/05/31 21:44:42, ahe wrote:
> > What does the RFC say about this situation?
> 
> I couldn't exactly figure it out, but my best guess is that a parser should
> reject the uri as invalid.  There are many invalid uris that my C++
> implementation does not reject, because I don't do full validation on all
> subcomponents.  So I think it is ok if I pass these invalid uris through.
> 
> There is a unit test that checks for an invalid escape sequence in a data uri.

> That test still passes.
> 
>
https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
> runtime/vm/uri.cc:395: // degenerate case, but it occurs when we resolve
> relative urls
> On 2016/05/31 21:44:42, ahe wrote:
> > I wouldn't call this a degenerate case. It's quite common to not provide a
> > scheme.
> 
> Corrected the comment.
> 
>
https://codereview.chromium.org/2011543002/diff/80001/runtime/vm/uri.cc#newco...
> runtime/vm/uri.cc:471: if (base.scheme != NULL && strcmp(base.scheme, "dart")
==
> 0) {
> On 2016/05/31 21:44:42, ahe wrote:
> > This is odd. Why is there a special case for the dart scheme?
> 
> Discussed offline.  Sometimes we resolve a relative uri inside of a dart:blah
> library and it is handled specially by the system.

[turnidge, 2016-06-02 19:30:01]

Description was changed from

==========
Canonicalize uris in C++ instead of Dart for the standalone embedder.

Adds Dart_DefaultCanonicalizeUrl() to the dart embedding api.

Motivation:

As we try to get source reloading working for the standalone embedder, things
get simpler if an isolate doesn't run Dart code while it is loading Dart code. 
We intend to solve this by moving the embedder tag handler calls to the service
isolate.  But making a blocking rpc into the service isolate whenever a url
needs to be canonicalized during parsing seems like it would slow things down
and make things complicated.  By moving canonicalization into C++, we avoid
this.
==========

to

==========
Canonicalize uris in C++ instead of Dart for the standalone embedder.

Adds Dart_DefaultCanonicalizeUrl() to the dart embedding api.

Motivation:

As we try to get source reloading working for the standalone embedder, things
get simpler if an isolate doesn't run Dart code while it is loading Dart code. 
We intend to solve this by moving the embedder tag handler calls to the service
isolate.  But making a blocking rpc into the service isolate whenever a url
needs to be canonicalized during parsing seems like it would slow things down
and make things complicated.  By moving canonicalization into C++, we avoid
this.

R=ahe@google.com, fschneider@google.com, johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a7d46eb5a687f5b12571966c1df17660877a2ff9
==========

[turnidge, 2016-06-02 19:30:03]

Committed patchset #9 (id:160001) manually as
a7d46eb5a687f5b12571966c1df17660877a2ff9 (presubmit successful).