dart2js: fix for https://github.com/dart-lang/sdk/issues/24412

dart2js: fix for https://github.com/dart-lang/sdk/issues/24412


The regression on some benchmarks was caused by
UnknownJavaScriptObject becoming alive.  Changing the test to 'is' reverts the regression.

Also make the cps_ir register UnknownJavaScriptObject when generating try-catch as that might snag a JavaScript error.

BUG= https://github.com/dart-lang/sdk/issues/24412
R=asgerf@google.com

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

Reverted.
Moved to: https://codereview.chromium.org/1363183003

[sra1, 2015-09-23 18:35:14]

sra@google.com changed reviewers:
+ asgerf@google.com

[asgerf, 2015-09-23 19:26:40]

The fix LGTM although I am concerned that we will now get a case for
UnknownJavaScriptObject in the CPS backend's getInterceptor method.

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:648:
register(glue.jsUnknownJavaScriptObjectClass);
I get that we ought to register these because they can flow into parts of the
system (although not into arbitrary user code, I hope). But why do they affect
interceptors? Do we ever call getInterceptor on a caught JS exception?

If yes, why do we do that when the extra case is so expensive?

If no, why is there a case for these classes in getInterceptor?

It seems to me we should just prohibit dynamic dispatch on this type of value
and let the interceptors focus on values that can flow into user code.

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/glue.dart (right):

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/glue.dart:277: 
whitespace

[sra1, 2015-09-24 00:24:45]

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:648:
register(glue.jsUnknownJavaScriptObjectClass);
On 2015/09/23 19:26:40, asgerf wrote:
> I get that we ought to register these because they can flow into parts of the
> system (although not into arbitrary user code, I hope). But why do they affect
> interceptors? Do we ever call getInterceptor on a caught JS exception?
> 
> If yes, why do we do that when the extra case is so expensive?

I am conflicted about this fix. Real programs need the extra expensive test.
This fix makes a few benchmarks faster than they would be if included in a web
app.  Maybe we should compile all benchmarks in a context that includes
dart:html.


Yes, caught exceptions can flow anywhere:

try {
  ...
} catch (e) {
  print(e);  // We must not fail to find a toString$0 method.
}

'...' could do anything since we use JS(...) in the runtime libraries. We try to
translate the JavaScript exception to a Dart exception but we don't always
succeed in recognizing obscure errors from various browsers.

The expensive 'x instanceof P.Object' test is needed in the interceptors when we
have native JavaScript objects (i.e. every web app) since we must not try to
interpret Dart objects as the JavaScript objects that represent them.

The cost of interceptors is why I suggest whole-program optimizations like
https://github.com/dart-lang/sdk/issues/23686

> 
> If no, why is there a case for these classes in getInterceptor?
> 
> It seems to me we should just prohibit dynamic dispatch on this type of value
> and let the interceptors focus on values that can flow into user code.

We are making rapid progress towards a kind of JavaScript interop where the
JavaScript values can flow unwrapped everywhere, and almost every web app will
use the interop to access some JavaScript libraries.

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/glue.dart (right):

https://codereview.chromium.org/1366673003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/glue.dart:277: 
On 2015/09/23 19:26:40, asgerf wrote:
> whitespace

Done.

[sra1, 2015-09-24 00:27:29]

Committed patchset #2 (id:20001) manually as
0bd5cc5f705339cd4f148a924abbb8a6f3f9127a (presubmit successful).

[sra1, 2015-09-24 01:14:26]

I have revert this and will redo as: https://codereview.chromium.org/1363183003