Dead code elimination removes event handlers (like onload for Image)

[GoogleCodeExporter]

What steps will reproduce the problem?

1. Compile the following code using advanced optimizations. This is a reduced 
test case from a real app that was experiencing a crash:

    function imageLoaded() {
      var c = document.createElement('canvas').getContext('2d');
      c.canvas.width = Test.IMAGE.width;
      c.canvas.height = Test.IMAGE.height;
      c.drawImage(Test.IMAGE, 0, 0);
      Test.ELEMENT = c.canvas;
      Test.ORIGIN = { x: 0, y: 0 };
    }

    /**
     * @constructor
     */
    function Test() {
    }

    Test.prototype.render = function() {
      var c = document.getElementById('canvas').getContext('2d');
      c.drawImage(Test.ELEMENT, Test.ORIGIN.x, Test.ORIGIN.y);
    };

    Test.IMAGE = new Image;
    Test.IMAGE.onload = imageLoaded;
    Test.IMAGE.src = 'mask.png';

    Test.ORIGIN = null;
    Test.ELEMENT = null;

    document.onmousedown = function() {
      new Test().render();
    };

What is the expected output? What do you see instead?

The expected code is code that is not only valid, but that contains the onload 
handler for the image. The actual output is missing the onload handler and 
tries to access properties off of null:

    new Image;
    document.onmousedown = function() {
      document.getElementById("canvas").getContext("2d").drawImage(null, null.x, null.y)
    };

What version of the product are you using? On what operating system?

This error is with the live version at http://closure-compiler.appspot.com/home.

Original issue reported on code.google.com by evan....@gmail.com on 9 Jul 2013 at 5:10

[GoogleCodeExporter]

In your example, onload is removed because it's never used. Advanced mode 
removes unused properties. Also, Test.ELEMENT is set to null, so the compiler 
correctly replaces it in the body of onmousedown.

In your original program, if you want to preserve a property that is not used, 
you must export it.

Original comment by dim...@google.com on 9 Jul 2013 at 5:20

• Changed state: Invalid

[GoogleCodeExporter]

To further clarify, the compiler doesn't understand that functions assigned to 
the 'onload' function are implicitly called. This does compile as expected:

    function imageLoaded() {
      var c = document.createElement('canvas').getContext('2d');
      c.canvas.width = Test.IMAGE.width;
      c.canvas.height = Test.IMAGE.height;
      c.drawImage(Test.IMAGE, 0, 0);
      Test.ELEMENT = c.canvas;
      Test.ORIGIN = { x: 0, y: 0 };
    }

    /**
     * @constructor
     */
    function Test() {
    }

    Test.prototype.render = function() {
      var c = document.getElementById('canvas').getContext('2d');
      c.drawImage(Test.ELEMENT, Test.ORIGIN.x, Test.ORIGIN.y);
    };

    Test.IMAGE = new Image;
    Test.IMAGE.addEventListener('load', imageLoaded, false);
    Test.IMAGE.src = 'mask.png';

    Test.ORIGIN = null;
    Test.ELEMENT = null;

    document.onmousedown = function() {
      new Test().render();
    };

Original comment by chadkill...@missouristate.edu on 10 Jul 2013 at 1:19

[GoogleCodeExporter]

@dimitris: are you sure? I thought the compiler preserved externed properties. 
See:
https://code.google.com/p/closure-compiler/source/browse/test/com/google/javascr
ipt/jscomp/RemoveUnusedClassPropertiesTest.java#73

Original comment by Nicholas.J.Santos on 11 Jul 2013 at 12:17

[GoogleCodeExporter]

You're right. There were no externs in the snippet and I didn't think about 
that.

Original comment by dim...@google.com on 11 Jul 2013 at 11:13

[GoogleCodeExporter]

Original comment by concavel...@gmail.com on 16 Jul 2013 at 11:45

• Changed state: New

[GoogleCodeExporter]

Basically, "onload" and "src" needs to be a setters with side-effects beyond a 
normal property set. Otherwise, it looks like Test.IMAGE is never used.  

"smart" name removal is the only pass that tries to connect the dots like this.

Original comment by concavel...@gmail.com on 17 Jul 2013 at 12:12

[GoogleCodeExporter]

Original comment by Nicholas.J.Santos on 26 Jul 2013 at 4:36

• Changed state: Accepted

[GoogleCodeExporter]

i posted a potential fix here:
http://codereview.appspot.com/11897044

Original comment by Nicholas.J.Santos on 26 Jul 2013 at 5:14

[GoogleCodeExporter]

This fix basically assumes that all external properties have global side 
effects?  This seems reasonable until we have support for getters/setters 
definitions.

Original comment by concavel...@gmail.com on 30 Jul 2013 at 3:28

[GoogleCodeExporter]

yep. this might be an over-zealous assumption.

Original comment by Nicholas.J.Santos on 31 Jul 2013 at 12:45

[GoogleCodeExporter]

What about @sideeffects to mirror @nosideeffects?

Original comment by evan....@gmail.com on 31 Jul 2013 at 1:14

[GoogleCodeExporter]

I'm also concerned about the effect this might have on dead-code elimination.

Original comment by chadkill...@missouristate.edu on 31 Jul 2013 at 12:52

[GoogleCodeExporter]

side-effects don't actually have much to do with it. for example, you could 
imagine an implementation of Foo that looked like this:

function Foo() { 
  setInterval(function () { if (this.src) { alert(this.src) } }.bind(this), 1000)
}

and it would be inappropriate to remove assignments to Foo's "src"

Original comment by Nicholas.J.Santos on 31 Jul 2013 at 2:02

[GoogleCodeExporter]

The real issue here seems to be the events. The compiler has no knowledge that 
assignments to certain properties can trigger events which then can have side 
effects. The "Image" class being the classic example:

var foo = new Image;
Image.onload = function() { /* my handler with side effects */};
Image.src = 'foo.png'; //assignment triggers events.

It seems the correct solution may be a new pass. However, other than hard 
coding properties on certain types, how do you know which property assignments 
trigger which events.

Assignment to the property without the event handler should be dead code. An 
event handler without the property assignment would also be dead code. Both of 
these may be problems that are complex to solve however.

Original comment by chadkill...@missouristate.edu on 31 Jul 2013 at 9:24

[GoogleCodeExporter]

I ran some numbers on this, and for some apps the "all external properties" 
assumption causes size increases on the order of 0.5%-1%, which seems big to me.

Original comment by blic...@google.com on 1 Aug 2013 at 5:00

[GoogleCodeExporter]

sad. yeah, that is probably too high. i will poke around at my own app and see 
if there are obvious ways we can make this better.

Original comment by Nicholas.J.Santos on 1 Aug 2013 at 10:27

[GoogleCodeExporter]

I believe it does have to do with side-effects.  

Nick's example: You can't remove property sets on extern property types, it 
simply doesn't make sense unless the entire object creation is side-effect 
free.  For Nick's "Foo" example "new Foo" has side-effects (the "this" has 
escaped), so its properties have to be retained (the properties are external so 
we must assume they are referenced).

Chad's example.  new Image is side-effect free, setting onload is not, modifies 
"this" side-effect,  but setting "src"  cause "this" to escape.  Even if there 
is no event handler this still has side-effects.  As example, It is a pretty 
common trick to use Image URLs to signal information to a server.  Any property 
that causes a network operation has to be assumed to have global side-effects.  
To avoid this we would have to have some mechanism to say "ignore the 
side-effects" here.

My general proposal to fix this longer term is this:
general properties are assumed to be side-effect free
properties gets/sets with side-effect should be define as getters/setters and 
annotated with appropriate side-effects.
In externs, support for Object.defineProperty is pretty reasonable (it is 
already an external property so using quoted property names is acceptable).

Creating common extern property collector is on my todo list as I want to 
support record types declarations and ES5/ES6 property declarations 
(Object.defineProperty/defineProperties/classes).  Once we have that having the 
side-effect function use that for determining property side-effects is 
reasonable.

I think this is the direction we should be heading.

Original comment by concavel...@gmail.com on 3 Aug 2013 at 9:12

[GoogleCodeExporter]

Don't we already have a mechanism for tracking if a function aliases "this"? 
shouldn't we be using that for "new Image"?

Original comment by Nicholas.J.Santos on 4 Aug 2013 at 8:09

[GoogleCodeExporter]

Does new Image itself have side-effects?  I thought the problem was "src"?  Or 
are you recommending it as a medterm workaround?

Original comment by concavel...@gmail.com on 12 Aug 2013 at 12:52

[GoogleCodeExporter]

I'm just saying that this would be a reasonable to model the situation, and I 
think it would give us the same results.

Original comment by Nicholas.J.Santos on 13 Aug 2013 at 2:01

[GoogleCodeExporter]

Issue tracking has been migrated to github. Please make any further comments on 
this issue through https://github.com/google/closure-compiler/issues

Original comment by blic...@google.com on 1 May 2014 at 6:31

• Added labels: Restrict-AddIssueComment-Nobody

[GoogleCodeExporter]

Original comment by blic...@google.com on 1 May 2014 at 6:34

• Changed state: Duplicate