Bugfix in DOM bindings: canonicalize URLs even if no ApplicationLoader is available

Bugfix in DOM bindings: canonicalize URLs even if no ApplicationLoader is available

When a dart application calls spawnUri, the DartVM will ask the embedder (in
this case the DOM bindings) to canonicalize a URL.
This is independent of script loading functionality but required so far an
instance of ApplicationLoader.

BUG=http://dartbug.com/11656
R=asiva@google.com, vsm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=1297

[kustermann, 2013-07-10 13:49:06]

We were just lucky that we didn't hit this issue in release mode.
(Reason: if tag == Dart_kCanonicalizeUrl we don't access any members and we
don't have vtable lookups, so we didn't dereference the "this" pointer [which is
NULL]). In debug mode hit the assertion.)

[kustermann, 2013-07-10 13:50:38]

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp
File DartApplicationLoader.cpp (left):

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#old...
DartApplicationLoader.cpp:90: return
DartDOMData::current()->applicationLoader()->libraryTagHandler(tag, library,
urlHandle);
(Sidenote: AFAIK the styleguide allows long lines, so I didn't shorten them.)

[siva, 2013-07-10 18:10:26]

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp
File DartApplicationLoader.cpp (right):

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:95: String libraryURL =
DartUtilities::toString(libraryURLHandle);
Why does this library to libraryURL translation have to happen eagerly. For the
case
url.startsWith("dart:") || .....
libraryURL is not used at all.

You could instead change the signature as:
static Dart_Handle CanonicalizeUrl(Dart_Handle urlHandle, String url,
Dart_Handle library);

and do the libraryURL translation inside CanonicalizeUrl if
url.startsWith("dart:") || url.startsWith("package:") is false.

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:104: return
DartDOMData::current()->applicationLoader()->libraryTagHandler(tag, urlHandle,
url, libraryURL);
Similarly change the signature of libraryTagHandler to
Dart_Handle libraryTagHandler(Dart_LibraryTag tag, Dart_Handle urlHandle, String
url, Dart_Handle library);

and inside libraryTagHandler do the translation only if
tag == Dart_kSourceTag is true.

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:115: // KURL have problems concating package:foo/bar
(without slashes right after colon)
KURL has problems......

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:118: libraryURL = "http://" + libraryURL.substring(8);
These magic numbers 8, 7 etc. make me uncomfortable,
can we use named constants instead which are more descriptive
e.g:
const char* kPackagePrefix = "package:";
const int kPackagePrefixLength = strlen(kPackagePrefix);

const char* kHttpPrefix = "http://";
const int kHttpPrefixLength = strlen(kHttpPrefix);

[siva, 2013-07-10 18:10:54]

Adding Vijay to the review list

[vsm, 2013-07-10 19:11:31]

On 2013/07/10 18:10:54, siva wrote:
> Adding Vijay to the review list

Martin - can you add a test / explain how this situation arises?  When is there
no existing ApplicationLoader?

Also, regarding style, you can run ./Tools/Scripts/check-webkit-style (from
third_party/WebKit) to lint.

[kustermann, 2013-07-11 12:34:11]

PTAL

> Martin - can you add a test / explain how this situation arises?  When is
there
> no existing ApplicationLoader?

Although this is the first time I'm reading/changing DOM bindings code, here's
what I found out during bug-hunting:

- DartController::startDart() will call 'DartController::loadScripts()' once the
DOM is loaded
   - will initialize the VM if necessary (passing a global callback for creating
new isolates -- "createPureIsolateCallback")
   - calls Dart_CreateIsolate()
   - attaches a libraryTagHandler for that isolate

If the DartVM wants to create a new isolate, it calls
"DartController::createPureIsolateCallback"
   - determines if an ApplicationLoader is necessary or not
   - if no ApplicationLoader is used (i.e. SpawnFunction + snapshot of
available)
      - get the application snapshot from parent isolate & load it
      - mark the isolate as runnable
   - if an ApplicationLoader is used
      - start "LoadIsolateSourcesTask()"
         - After the main script is loaded (see in
'DartApplicationLoader::load()') 
              - attaches a libraryTagHandler for that isolate
              - call 'setApplicationLoader(this)'
              - call Dart_LoadScript()
         - After all the remaining scripts are loaded
              - make a snapshot of the app (can be used if this one will do a
'SpawnFunction')
              - call DartApplicationLoader::callEntryPoint()
                  - call
DartController.cpp.ApplicationLoadedCallback::handleEvent() 
                       - marks isolate as runnable
                  - Dart_Invoke 'main()'
                  - DartDOMData::current()->setApplicationLoader(0);

So after all scripts have been loaded and 'main()' was invoked, the
DartApplicationLoader will be set to NULL.
If the application calls 'spawnUri(URL)' at this point, the DartVM will try to
canonicalize the 'URL' by calling the attached libraryTagHandler.
And here's the point: the libraryTagHandler cannot access the
DartApplicationLoader (since it's NULL).
-> That's why we hit the assertion.
-> This CL only changes one thing: moving the canonicalization code out of an
instance method into a static method of DartApplicationLoader.

I don't know if the DartVM supports lazy loading and if so, how it's
implemented. This could be an issue (i.e. if the app tries to lazy load a
library at some point in the future [and the DartApplicationLoader is no longer
there ...]).

Hope this explains this CL.


> Also, regarding style, you can run ./Tools/Scripts/check-webkit-style (from
> third_party/WebKit) to lint.

Thank you for the hint.

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp
File DartApplicationLoader.cpp (right):

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:95: String libraryURL =
DartUtilities::toString(libraryURLHandle);
On 2013/07/10 18:10:26, siva wrote:
> Why does this library to libraryURL translation have to happen eagerly. For
the
> case
> url.startsWith("dart:") || .....
> libraryURL is not used at all.
> 
> You could instead change the signature as:
> static Dart_Handle CanonicalizeUrl(Dart_Handle urlHandle, String url,
> Dart_Handle library);
> 
> and do the libraryURL translation inside CanonicalizeUrl if
> url.startsWith("dart:") || url.startsWith("package:") is false.

Done.

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:104: return
DartDOMData::current()->applicationLoader()->libraryTagHandler(tag, urlHandle,
url, libraryURL);
On 2013/07/10 18:10:26, siva wrote:
> Similarly change the signature of libraryTagHandler to
> Dart_Handle libraryTagHandler(Dart_LibraryTag tag, Dart_Handle urlHandle,
String
> url, Dart_Handle library);
> 
> and inside libraryTagHandler do the translation only if
> tag == Dart_kSourceTag is true.

Done.

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:115: // KURL have problems concating package:foo/bar
(without slashes right after colon)
On 2013/07/10 18:10:26, siva wrote:
> KURL has problems......

Done.

https://codereview.chromium.org/18024004/diff/1/DartApplicationLoader.cpp#new...
DartApplicationLoader.cpp:118: libraryURL = "http://" + libraryURL.substring(8);
On 2013/07/10 18:10:26, siva wrote:
> These magic numbers 8, 7 etc. make me uncomfortable,
> can we use named constants instead which are more descriptive
> e.g:
> const char* kPackagePrefix = "package:";
> const int kPackagePrefixLength = strlen(kPackagePrefix);
> 
> const char* kHttpPrefix = "http://";
> const int kHttpPrefixLength = strlen(kHttpPrefix);

Done.

[vsm, 2013-07-12 00:48:22]

lgtm

Thanks for the explanation!

[siva, 2013-07-12 01:05:44]

lgtm

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h
File DartApplicationLoader.h (right):

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h#n...
DartApplicationLoader.h:70: static Dart_Handle
libraryTagHandlerCallback(Dart_LibraryTag, Dart_Handle library, Dart_Handle
urlHandle);
Dart_LibraryTag tag,

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h#n...
DartApplicationLoader.h:72: Dart_Handle libraryTagHandler(Dart_LibraryTag,
Dart_Handle library, Dart_Handle urlHandle, String url);
Dart_LibraryTag tag,

[kustermann, 2013-07-12 16:02:21]

Thank you for the review.

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h
File DartApplicationLoader.h (right):

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h#n...
DartApplicationLoader.h:70: static Dart_Handle
libraryTagHandlerCallback(Dart_LibraryTag, Dart_Handle library, Dart_Handle
urlHandle);
On 2013/07/12 01:05:44, siva wrote:
> Dart_LibraryTag tag,

I didn't want to remove it, but I had a hard time arguing with the webkit style
checker:

$ ./Tools/Scripts/check-webkit-style 
Source/bindings/dart/DartApplicationLoader.h:70:  The parameter name "tag" adds
no information, so it should be removed.  [readability/parameter_name] [5]
Source/bindings/dart/DartApplicationLoader.h:72:  The parameter name "tag" adds
no information, so it should be removed.  [readability/parameter_name] [5]

I think it outputs this warning if the name of the parameter is a substring of
the typename -- or something similar.

To keep the files free of style warnings, I'll let it as it is now.

[kustermann, 2013-07-12 16:28:03]

Committed patchset #2 manually as r1297 (presubmit successful).

[siva, 2013-07-12 16:58:09]

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h
File DartApplicationLoader.h (right):

https://codereview.chromium.org/18024004/diff/12001/DartApplicationLoader.h#n...
DartApplicationLoader.h:70: static Dart_Handle
libraryTagHandlerCallback(Dart_LibraryTag, Dart_Handle library, Dart_Handle
urlHandle);
That is a very weird style guideline. Thanks for the explanation..

On 2013/07/12 16:02:21, kustermann wrote:
> On 2013/07/12 01:05:44, siva wrote:
> > Dart_LibraryTag tag,
> 
> I didn't want to remove it, but I had a hard time arguing with the webkit
style
> checker:
> 
> $ ./Tools/Scripts/check-webkit-style 
> Source/bindings/dart/DartApplicationLoader.h:70:  The parameter name "tag"
adds
> no information, so it should be removed.  [readability/parameter_name] [5]
> Source/bindings/dart/DartApplicationLoader.h:72:  The parameter name "tag"
adds
> no information, so it should be removed.  [readability/parameter_name] [5]
> 
> I think it outputs this warning if the name of the parameter is a substring of
> the typename -- or something similar.
> 
> To keep the files free of style warnings, I'll let it as it is now.