Fix bug where Script files that only contained patches were lost from the list of LoadedScripts() T…

Fix bug where Script files that only contained patches were lost from the list of LoadedScripts() This resulted in incorrect stack traces for int.parse Many Script files containing patches also contained non-patch classes or top level members and so they still worked.

BUG=
R=asiva@google.com

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

[Jacob, 2013-09-05 04:09:59]

This CL fixes
https://code.google.com/p/dart/issues/detail?id=12360

Let me know if there is a simpler way to correctly determine all Scripts defined
by a library that contains patch files.

[turnidge, 2013-09-05 18:28:56]

On 2013/09/05 18:18:20, Jacob wrote:

DBC

I will leave the review to asiva/hausner.  One thing I noticed in looking over
the code is that there doesn't seem to be any coverage of this stuff in our
tests.

If the CL stays as is, It would be nice to see a test in
debugger_api_impl_test.cc that calls Dart_GetScriptUrls with a patch script to
test this case.

[siva, 2013-09-05 18:35:11]

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6806
runtime/vm/object.cc:6806: Script& function_script = Script::Handle();
Class& patch_cls = Class::Handle();
Script& patch_script = Script::Handle();

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6822
runtime/vm/object.cc:6822: }
I don't think it is necessary to go over all the functions in the class to add
the patch script. It should be sufficient to refer to the patch_class field if
one exists.

cls = Class::Cast(entry);
owner_script = cls.script();
patch_cls = cls.patch_class();
if (!patch_class.IsNull()) {
  patch_script = patch_class.script();
  AddScriptIfUnique(scripts, patch_script);
}

[hausner, 2013-09-05 21:23:51]

I second Siva's comment.

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6783
runtime/vm/object.cc:6783: void AddScriptIfUnique(const GrowableObjectArray&
scripts, Script& candidate) {
static

[Jacob, 2013-09-05 23:58:18]

On 2013/09/05 21:23:51, hausner wrote:
> I second Siva's comment.
> 
> https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6783
> runtime/vm/object.cc:6783: void AddScriptIfUnique(const GrowableObjectArray&
> scripts, Script& candidate) {
> static

I added a test to dart_api_impl_test as it already had a test with a comment to
test that all source files show up in the library.

[Jacob, 2013-09-05 23:58:24]

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6806
runtime/vm/object.cc:6806: Script& function_script = Script::Handle();
On 2013/09/05 18:35:11, siva wrote:
> Class& patch_cls = Class::Handle();
> Script& patch_script = Script::Handle();

Done.

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6822
runtime/vm/object.cc:6822: }
On 2013/09/05 18:35:11, siva wrote:
> I don't think it is necessary to go over all the functions in the class to add
> the patch script. It should be sufficient to refer to the patch_class field if
> one exists.
> 
> cls = Class::Cast(entry);
> owner_script = cls.script();
> patch_cls = cls.patch_class();
> if (!patch_class.IsNull()) {
>   patch_script = patch_class.script();
>   AddScriptIfUnique(scripts, patch_script);
> }

That is simpler. Done.

[Jacob, 2013-09-06 00:56:58]

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/23982002/diff/1/runtime/vm/object.cc#newcode6783
runtime/vm/object.cc:6783: void AddScriptIfUnique(const GrowableObjectArray&
scripts, Script& candidate) {
On 2013/09/05 21:23:51, hausner wrote:
> static

Done.

[siva, 2013-09-06 15:33:51]

lgtm

https://codereview.chromium.org/23982002/diff/11001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/23982002/diff/11001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6794: Script& script_obj = Script::Handle();
Move the handle creation below the null check so we don't create a handle if it
is null.

https://codereview.chromium.org/23982002/diff/11001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:6798: return;
Seems like duplicate checks:

candidate.IsNull() is the same as candidate.raw() == Object::null()

[Jacob, 2013-09-06 17:54:10]

Committed patchset #4 manually as r27254 (presubmit successful).