638838 - Cannot use 'var onmessage = function() { ... }' in workers post-b12

Status: RESOLVED WORKSFORME

(Core :: DOM: Workers, defect, P5)

Description

[Michel Gutierrez]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b11) Gecko/20100101 Firefox/4.0b11

One of my users reported an issue occurring on Firefox 4.0b12 while it was working on 4.0b11

I narrowed down the problem to the inability to post a message to a worker created from nsIWorkerFactory. More exactly the message is never received by the worker.

I wrote a minimal extension (attached to this bug entry) to demonstrate the issue:

Launcher code:
var workerFactory = Components.classes["@mozilla.org/threads/workerfactory;1"].createInstance(Components.interfaces.nsIWorkerFactory);
var worker = workerFactory.newChromeWorker("chrome://bugworker/content/bugworker.js");
worker.postMessage({});

Worker code:
dump("[bugworker] loading\n");
var onmessage = function(event) {
	dump("[bugworker] onmessage\n");
};
dump("[bugworker] loaded\n");




Reproducible: Always

Steps to Reproduce:
1. install the attached extension
2. visit chrome://bugworker/content/bugworker.xul
3. watch the console
Actual Results:  
From Firefox 4.0b11 (working as expected), console output is:
[bugworker] loading
[bugworker] loaded
[bugworker] onmessage

On Firefox 4.0b12 (message never received by the worker):
[bugworker] loading
[bugworker] loaded



Expected Results:  
On Firefox 4.0b12, the message should be received.

Comment 1

[Michel Gutierrez]

Attachment: Minimal extension to demonstrate the issue

After installing the extension, visit chrome://bugworker/content/bugworker.xul and watch the console.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ben, can you look at this and figure out whether this was an intentional change or something that's seriously broken for chrome workers?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ugh... This isn't just for ChromeWorkers...

It looks like some JS engine changes have made this code not work:

  var onmessage = function(event) { ... }

Workaround is simple, just change that to:

  onmessage = function(event) { ... }

Somehow specifying the var there means that our addProperty hook no longer gets called here:

https://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorker.cpp#953

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

All signs are pointing to bug 632003.

Comment 5

[Jeff Walden [:Waldo]]

Maybe we need an addProperty call after the js_DNP in JSOP_DEFVAR in the interpreter?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, the var part there...  I always thought that the AddProperty thing was pretty weird, but I think jst claimed that at least for DOM event receivers this was needed for web compat?

Comment 7

[Jeff Walden [:Waldo]]

jst was saying that workers use IDL to define properties, so it's possible there's a "onmessage" property on Object.getPrototypeOf(this) in workers.  If so, before we'd not define a var then (because we found the property), and now we do (because we found it, but it wasn't on the global object directly), so the shadowing var now would hide the other one and result in the event-handling special behavior happening.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It's not the AddProperty hook that has changed. Apparently it is no longer needed (which is weird - we had to add it specifically to solve this problem eons ago, something must have changed in XPConnect since).

Reversing the patch in bug 632003 fixes this bug.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I chatted with mrbkap about this, he's convinced that regular DOM event handlers (onclick, onunload, etc.) are unaffected because of the way DOMClassInfo handles this (there's no IDL for this and it works by resolving on the object itself, not its prototype).

Are there other cases like workers, though? It would have to be an IDL-specified global object where the property being asssigned lives on the prototype, not the actual object.

Given that we can fix workers separately (after 4.0) and suggest a simple workaround in the meantime (don't use 'var') I don't think this is an emergency.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Are there other cases like workers, though?

Not for |var| usage, I think, since that only affects global objects.  That means Window (which uses the classinfo code), workers, js components (which iirc don't use IDL for the global), and XBL (where |var| in fields treats the element as the global, but it's generally all kinda broken).  Oh, sandboxes, where again I think we don't use IDL for the global _and_ we don't have the on* stuff.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, and, I think changing the way we define properties on global objects with var vs. without var shouldn't have been changed in b12. Isn't that really really late for such a change?

Of course, we would have caught the problem with a test, but it has literally never crossed my mind to try defining one of these message handlers as a var (what nasty syntax!). In any case I'll add a test for this once we fix it.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Consensus among bz, shaver, and waldo is that this should be WONTFIX. Workaround is to not use 'var'. I'll file a followup to remove the AddProperty hook that isn't actually doing anything for us now.

Comment 13

[Boris Zbarsky [:bzbarsky]]

More precisely, using |var| there should not work, per spec.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #10)
> > Are there other cases like workers, though?
> 
> Not for |var| usage, I think, since that only affects global objects.

Is that true even for:

<input onclick="var onmouseup = x;">

?

Another issue is that HTML5 uses IDL to add all onfoo properties to elements and global objects. This is something that we really want to implement since it allows feature detection using things like |if ("onbar" in myelement) { ... }|

That's of course not something we need to worry about for FF4 though.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Is that true even for:
> <input onclick="var onmouseup = x;">

Yes.  That's compiled using nsJSContext::CompileEventHandler, which which passes the string to JS_CompileUCFunctionForPrincipalsVersion (hence compiles it as a function body).  So |var| in there will define variables on the current function invocation's Call object.

> Another issue is that HTML5 uses IDL to add all onfoo properties

Right; that's what comment 13 is based on.  If the on* are declared as properties in IDL and WebIDL puts properties on the prototype (which it does), then |var onfoo| will just shadow the IDL-declared |foo| instead of setting it.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Another issue is that HTML5 uses IDL to add all onfoo properties
> 
> Right; that's what comment 13 is based on.  If the on* are declared as
> properties in IDL and WebIDL puts properties on the prototype (which it does),
> then |var onfoo| will just shadow the IDL-declared |foo| instead of setting it.

That might not be compatible with the web then :( I would even say that it likely isn't.

What does

function onmessage(e) {
  ...
}

do in workers? I bet there are loads of pages out there that does

function onload() {
  ...
}

on the web today.

Comment 17

[Jeff Walden [:Waldo]]

data:text/html,<script>function onload() { alert('hi'); }</script>

alerts in a nightly.  Why, I don't actually know, to be honest, but that's not actually "broken" right now.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In existing code we don't use IDL to define onfoo properties (other than in a few instances, like workers and XMLHttpRequest).

Instead we simply look at for "expando" properties set on the JS object for the window when we go looking for event handlers to fire.

However HTML5 changes that such that IDL *is* used. And we want to follow that for several reasons. My concern is that that, in combination with the changes in bug 632003, would break the web.

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> In existing code we don't use IDL to define onfoo properties (other than in a
> few instances, like workers and XMLHttpRequest).
> 
> Instead we simply look at for "expando" properties set on the JS object for the
> window when we go looking for event handlers to fire.

We always should look up "onload" (e.g.) starting with the window object.

Looking for the property when firing the event, and defining the property based on markup, are two different things.

> However HTML5 changes that such that IDL *is* used.

How, exactly? Do you mean to bind a property named 'onload' based on <body onload="...">, HTML5 says something other than define an "own" property of the window object?

/be

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #17)
> data:text/html,<script>function onload() { alert('hi'); }</script>
> 
> alerts in a nightly.  Why, I don't actually know, to be honest, but that's not
> actually "broken" right now.

Why wouldn't that work?

IE had some strangeness where function onload(){} shadowed the event handler induced by <body onload='...'> but no Netscape or Mozilla browser ever did that.

/be

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > However HTML5 changes that such that IDL *is* used.
> 
> How, exactly? Do you mean to bind a property named 'onload' based on <body
> onload="...">, HTML5 says something other than define an "own" property of the
> window object?

HTML5 uses normal WebIDL to define all the on* properties. So it contains:

interface Window {
  ...
  attribute Function onload;
  ...
};

WebIDL defines what prototype chain this maps to, and since in all implementations (possibly except for IE, don't know) the above IDL has mapped to onload being a getter+setter on some object on the prototype chain, rather than on the object itself, I imagine that that is what WebIDL will say.

Comment 22

[Michel Gutierrez]

Thanks guys for sorting that out.

I suggest fixing the web worker documentation at https://developer.mozilla.org/en/using_web_workers where the example for echoing a message from the worker is written as:

1	var onmessage = function(e) {
2	  postMessage(e.data);
3	};

Comment 23

[Jeff Walden [:Waldo]]

I fixed/clarified/expanded it, thanks.  Do note that anyone can create an account on MDN to make changes to articles (or even to write new ones), so nothing prevents you from fixing any mistakes you happen to notice in them yourself.  :-)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

http://mozilla.pettay.fi/moztests/w.html returns PASS on FF4, Opera 11 and
Chrome 9.  
FAIL&PASS on FF3.6.

The FAIL case is testing 'var onmessage =' and the PASS case 'onmessage ='.



For post FF work I'm still worried about the change for the reasons
sicking mentioned.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Jonas, IDL attributes map to getter/setter pairs on the prototype in IE and Gecko.  They map to own properties in JSC/V8.  I don't recall what Opera does.  This is the case for all properties defined in IDL, not just on*.

WebIDL was going to define the IE/Gecko behavior here as correct; at least the JSC folks indicated they would switch to it.

I'm a little surprised the |function onload()| works while |var onload| does not against Window (the latter in both 4.0 and 3.6).  Does the |function| case call addProperty or something?

In any case, before we start implementing this stuff for Window we should write some exhaustive tests and compare cross-browser behavior.

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> I'm a little surprised the |function onload()| works while |var onload| does
> not against Window (the latter in both 4.0 and 3.6).

I suppose this is because JSOP_DEFVAR case in the interpreter calls js_DefineNativeProperty(obj) while JSOP_DEFFUN calls obj->defineProperty() allowing to override js_DefineNativeProperty with a custom operation. We should definitely have the uniform behavior for both cases. I will dig into this farther.

Comment 27

[Igor Bukanov]

The reason function onload() works while var onload = function()  does not is that in the former case the runtime calls js_DefineNativeProperty with the function as a value while for the latter we pass JSVAL_VOID to the define method. Only later the bytecode will set the value of the already existing property.

This means that AddProperty hook for the window, http://mxr.mozilla.org/mozilla-central/ident?i=NS_IMETHODIMP , does nothing for the var case due to the checks in http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7649 that skips the void value:

7649   if ((::JS_TypeOfValue(cx, *vp) != JSTYPE_FUNCTION && !JSVAL_IS_NULL(*vp)) ||
7650       !JSID_IS_STRING(id) || id == sAddEventListener_id) {
7651     return NS_OK;
7652   }
7653 
7654   PRBool did_compile; // Ignored here.
7655 
7656   return RegisterCompileHandler(wrapper, cx, obj, id, PR_FALSE,
7657                                 JSVAL_IS_NULL(*vp), &did_compile);

Comment 28

[Igor Bukanov]

I reopen the bug so the code treats var onload and function onload uniformly.

Comment 29

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

And there is also http://mozilla.pettay.fi/moztests/w.html

The behavior is the same also in other browsers.