Fix this->window confusion in DevMode

Fix for Issue 3069.

[scottb]

http://gwt-code-reviews.appspot.com/1620805/diff/1/dev/core/src/com/google/gw...
File dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java (right):

http://gwt-code-reviews.appspot.com/1620805/diff/1/dev/core/src/com/google/gw...
dev/core/src/com/google/gwt/dev/shell/ModuleSpaceOOPHM.java:77: body =
body.substring(0, i + 1) + fun + body.substring(i + 1);
Yuck.  Have you guys already talked about just throwing the exception on the
Java side?  The rewritten hosted mode code that calls into the browser channel
could do the check itself without going over the wire.

[stephenh]

> > body.substring(0, i + 1) + fun + body.substring(i + 1);
> Yuck.

Yeah. :-)

> Have you guys already talked about just throwing the exception on the
> Java side?

I think that is what John referenced on comment 12 in the issue:

"we cannot guarantee NPEs in production mode (typically you will get a JS 
exception, but not always as methods can be devirtualized, made static, and
inlined).
We could detect some subset of these in DevMode, but we couldn't catch all of
them.  
In particular, JSO methods are frequently going to run entirely in the browser
so we wouldn't get hooks to check for NPEs unless we rewrite the JS code to
check and 
manually throw an NPE (which seems hard to get right)."

I'm guessing here, but my understanding was that "null.doFoo()" could actually
work in web mode if it ended up being "doFoo(null)" and the body of "doFoo"
never used "this"/"this$static".

So, instead of adding a Java-side null check to all JSO instance method
invocations (which might cause more devmode NPEs than webmode would), I thought
that this (admittedly very hacky) approach of just putting the null back and
letting the JS run as-is would most match what will happen in webmode.

I think.

[jat]

But the point is it is almost certainly an error to call a method on a null
value, and catching it in DevMode would be better than replicating the behavior
of prod mode in this case.

[scottb]

If there were a very elegant way to make the dev mode behavior always consistent
with the production mode behavior, I think I'd be happier.  But this seems
pretty gruesome.

Failing in dev mode rather than supporting call-instance-through-null behavior
would seem more in the spirit of GWT.

[stephenh]

Agreed it's almost certainly an error, I just assumed drifting DevMode to be
stricter than prod would end up as another bug report down the road ("hey, this
failed in devmode, but not in my production app").

Perfectly willing to defer to you guys though; my original spike for this did
use a Java side null check in the hosted mode JSO rewriting...unfortunately I
don't think I kept a branch/stash of it. I'll get around to resurrecting it and
update the review.

[stephenh]

Okay, updated the patch. Within the rewritten JSNI methods, before making the
jump to javascript, we check for null instances (assuming the method isn't
static).

[stephenh]

http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google...
File dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java (right):

http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google...
dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java:35: }
I tried a few other places to put this (like util.Util or another class in
shell.rewrite), but CompilingClassLoader wasn't having it.

[scottb]

LGTM w/ nit

http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google...
File dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsniMethods.java
(right):

http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google...
dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsniMethods.java:270:
loadThis();
If make it "Object checkNotNull(Object)" and return the passed-in object, you
can just inject the call right here.

[stephenh]

http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google...
File dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsniMethods.java
(right):

http://gwt-code-reviews.appspot.com/1620805/diff/8001/dev/core/src/com/google...
dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsniMethods.java:270:
loadThis();
> If make it "Object checkNotNull(Object)" and return the passed-in object, you
> can just inject the call right here.

Makes sense, done.

[scottb]

LGTM

http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/googl...
File dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java (right):

http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/googl...
dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java:34: throw new
NullPointerException("JSO reference was null");
Oops, I meant to say to make this "instance" rather than "reference".

[stephenh]

http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/googl...
File dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java (right):

http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/googl...
dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java:34: throw new
NullPointerException("JSO reference was null");
> Oops, I meant to say to make this "instance" rather than "reference".

I originally used instance, but then changed it because an instance cannot be
null--so I used reference instead. Could maybe use variable. Np if you still
prefer instance though.

[scottb]

http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/googl...
File dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java (right):

http://gwt-code-reviews.appspot.com/1620805/diff/12001/dev/core/src/com/googl...
dev/core/src/com/google/gwt/dev/shell/JavaScriptHost.java:34: throw new
NullPointerException("JSO reference was null");
"instance" in the sense of "instance method".  It's the instance upon which the
method is being invoking.  See JMethodCall.instance field, for example.

[stephenh]

> "instance" in the sense of "instance method"

Done.

[rdayal]

On 2012/02/28 05:16:42, stephenh wrote:
> > "instance" in the sense of "instance method"
> 
> Done.

submitted.

[rdayal]

On 2012/04/09 18:33:00, rdayal wrote:
> On 2012/02/28 05:16:42, stephenh wrote:
> > > "instance" in the sense of "instance method"
> > 
> > Done.
> 
> submitted.

We had to roll this back because it broke a bunch of test code in the Google
environment. I think that this patch is in the right, but the test code was not
carefully written, so legit failures revealed by this patch actually are
blocking tests from completing successfully. 

I think this patch is important, but we'll probably need to hide this behavior
behind a switch for backwards compatibility. I'm inclined not to put this into
2.5. 

Stephen, is that ok with you?

[stephenh]

> We had to roll this back because it broke a bunch of test code

Cool, I saw that, was wondering what had happened.

> I think that this patch is in the right, but the test code was not
> carefully written

Good, I was hoping you thought the patch was still right.

> Stephen, is that ok with you?

That is fine with me, AFAIK it's not actively affecting me right now.

Thanks for following up on it.

[jat]

On Tue, May 8, 2012 at 2:38 PM, <stephen.haberman@gmail.com> wrote:

>
>  We had to roll this back because it broke a bunch of test code
>>
>
> Cool, I saw that, was wondering what had happened.
>
>
>  I think that this patch is in the right, but the test code was not
>> carefully written
>>
>
> Good, I was hoping you thought the patch was still right.


I would add that since internal test code relies on being able to call
through null values in some cases, it seems likely that external code we
can't test also does so.  So, this would have to be treated as a breaking
change.  That doesn't mean that we can't do it, just that it needs to be
carefully considered and communicated about when it lands.

-- 
John A. Tamplin
Software Engineer (GWT), Google

[stephenh]

> it seems likely that external code we can't test also does so

True, although my assumption is that since this is a DevMode-only behavior (null
becomes window), we're actually doing everyone a favor by fixing the behavior to
match web mode, and in doing so highlighting previously-missed/potential bugs in
their application code/test code.

So, it seems like the consideration could be less careful than, say, a semantics
change where people might still want the previous behavior.

(I'm assuming for this change that the previous behavior really isn't all that
desirable, once you know about it/update your code/tests accordingly...)

[jat]

On Tue, May 8, 2012 at 3:23 PM, <stephen.haberman@gmail.com> wrote:

> True, although my assumption is that since this is a DevMode-only
> behavior (null becomes window), we're actually doing everyone a favor by
> fixing the behavior to match web mode, and in doing so highlighting
> previously-missed/potential bugs in their application code/test code.
>
> So, it seems like the consideration could be less careful than, say, a
> semantics change where people might still want the previous behavior.
>
> (I'm assuming for this change that the previous behavior really isn't
> all that desirable, once you know about it/update your code/tests
> accordingly...)
>

I agree, but asking anyone to have to make changes in working code as a
consequence of upgrading increases the cost, even if they are good changes.

-- 
John A. Tamplin
Software Engineer (GWT), Google

[stephenh]

> I agree, but asking anyone to have to make changes in working code

Two things, one is that I don't think any code that relies on this
behavior could really be called "working". It's dev mode mistakingly
acting differently than web mode.

> as a consequence of upgrading increases the cost, even if they are
> good changes.

The other is that (disclaimer, this isn't really a discussion for the
bug tracker, and is just my personal opinion) I think this extreme
dedication to backwards compatibility at some point hurts more than it
helps.

Yes, it means users have 0-change upgrades, which is great if you do
an upgrade every month (or on every commit, e.g. internally to Google).

But after years of 0-change upgrades, I think the burden of backwards
compatibility starts to wear on a codebase. Of course every release
can't be a fresh start, but I think there needs to be a compromise
that's less-than-every-release but more-than-never.

Furthering my musing that is kind of silly to do in an issue, I think
Google's internal "all upstream/downstream projects always build from
trunk" encourages codebases to grow stale as they can never break any
project ever.

Let's have some forks, branches, or something, that at least
occasionally lifts this burden of backwards compatibility for those of
us who are just fine with a yearly-ish/non-bugfix release that has
breaking changes in the name of a better, cleaner codebase.

[rdayal]

Hey Stephen,

Thanks for the feedback. Replies inline:


On Tue May 08 23:22:49 GMT-400 2012, Stephen Haberman <
stephen.haberman@gmail.com> wrote:

>
> > I agree, but asking anyone to have to make changes in working code
>
> Two things, one is that I don't think any code that relies on this
> behavior could really be called "working". It's dev mode mistakingly
> acting differently than web mode.
>

Agreed, but if you have a ton of internal code that is written in such a
way that flipping on this behavior causes a ton of failures, then it's
going to take time to fix them all. I totally agree that they should be
fixed, but it's going to take a non-trivial amount of time.

I'd rather spend that time working on SuperDevMode and moving people over
to that, where this whole issue of differing implementations between web
mode and dev mode goes away.

>
> > as a consequence of upgrading increases the cost, even if they are
> > good changes.
>
> The other is that (disclaimer, this isn't really a discussion for the
> bug tracker, and is just my personal opinion) I think this extreme
> dedication to backwards compatibility at some point hurts more than it
> helps.
>
> Yes, it means users have 0-change upgrades, which is great if you do
> an upgrade every month (or on every commit, e.g. internally to Google).
>
> But after years of 0-change upgrades, I think the burden of backwards
> compatibility starts to wear on a codebase. Of course every release
> can't be a fresh start, but I think there needs to be a compromise
> that's less-than-every-release but more-than-never.
>

I completely agree that supporting backwards compatibility all of the time
wears on the codebase. However, if you want to suddenly stop supporting
this, you need a clean break. Maybe that clean break is defining a new
user-level library for GWT; maybe it's SuperDevMode, or something else -
but, we do need to define this break at some point soon, and allow for the
breaking of backwards compatibility. I'd also agree that we should be less
conservative going forward.

I just don't think this issue is one where we should start to break
backwards compatibility.


> Furthering my musing that is kind of silly to do in an issue, I think
> Google's internal "all upstream/downstream projects always build from
> trunk" encourages codebases to grow stale as they can never break any
> project ever.
>

:) That's a much larger discussion.


>
> Let's have some forks, branches, or something, that at least
> occasionally lifts this burden of backwards compatibility for those of
> us who are just fine with a yearly-ish/non-bugfix release that has
> breaking changes in the name of a better, cleaner codebase.
>

Stay tuned. There are going to be some major changes in the coming weeks
that allow for much more flexibility than is currently offered to the
external community.

To circle back, let's not put this in to 2.5. I do suggest enabling the
behavior behind a flag, and we can push people to switch to the new
behavior. Internally, we can then tell people the stricter (correct)
behavior is going to become the default, and get rid of the flag. Stephen,
what do you think? Would you be amenable to adding the code for the flag?

[stephenh]

> I'd rather spend that time working on SuperDevMode and moving people
> over to that, where this whole issue of differing implementations
> between web mode and dev mode goes away.

+1 to that.

> I just don't think this issue is one where we should start to break
> backwards compatibility.

That is fine with me. This was issue that wasn't really directly affecting me,
just seemed fun to work on.

> Stephen, what do you think? Would you be amenable to adding the code
> for the flag?

Sure, I'll see what I can do.

- Stephen