MozAfterPaint event is unavailable from content by default.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=80a12187de61

Backed out for mochitest-3 failures in test_interfaces.html (Windows, so far): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1f6b5cb3fe3b&jobname=mochitest-3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/32410b741451

Although this may just be bug 909508 again, sigh thought that had been fixed :-/ If this does just need a clobber, please modify the CLOBBER file when relanding :-)

Scratch that, B2G failures too: https://tbpl.mozilla.org/php/getParsedLog.php?id=28154860&tree=Mozilla-Inbound

Fixed orange on Windows and B2G.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=854609de215c

Comment on attachment 808143 [details] [diff] [review] patch v2 Please don't add code to nsPresContext::FireDOMPaintEvent. That is a hot code path, and we should make it faster, not slower. Couldn't you ensure those ctors exist when the relevant prescontext is created or something?

Try run: https://tbpl.mozilla.org/?tree=Try&rev=212bc868b476

Comment on attachment 809496 [details] [diff] [review] patch v3 Actually, this starts to look ugly enough, that we should think about https://bugzilla.mozilla.org/show_bug.cgi?id=909340#c8

Rebased patches of bug 909340.

Try runs: https://tbpl.mozilla.org/?tree=Try&rev=d3abd645e40e https://tbpl.mozilla.org/?tree=Try&rev=456ef94eddb0

Ping?

Yeah, sorry about the delays. I've actually started looking at this, but I don't yet completely understand why we need to decouple to be able to hide interface objects.

Oops, quoting the description from the previous bug: https://bugzilla.mozilla.org/show_bug.cgi?id=909340#c2 > Without this patch, aDefineOnGlobal will be effectively ignored if > GetProtoObject is called before GetConstructorObject. And it is > unpredictable when GetConstructorObject is called because GlobalResolve > calls it lazily.

GetProtoObject will fill protoAndIfaceArray and it will call CreateInterfaceObjects with aDefineOnGlobal = true which will define interface objects on the global object. If protoAndIfaceArray is already filled, GetConstructorObject will not even call CreateInterfaceObjects. Also, interface objects are already defined on the global object by GetProtoObject. Therefore aDefineOnGlobal will have no effect. We can't avoid creating the interface object in GetProtoObject because the prototype object refers it via .constructor property. So I introduced mDefined bitmap to control whether we have tried to define the interface object on the global object.

For ease of review.

But why can't we just pass the right value for aDefineOnGlobal in GetProtoObject/GetConstructorObject in the first place (through an optional NativeType::DefineInterfaceOnGlobal() or something)?

GetProtoObject does not take aDefineOnGlobal argument. Even if we changed the signature, GetProtoObject will be called from XXXBinding::Wrap which will be called whenever scripts need the interface object or the prototype object. XBL touches the XULElement interface on initialize (it was bug 909340). The PaintRequest interface will be touched when a MozAfterPaint event is dispatched (this bug). It's very complicated (if at all possible) to ensure that the interface object is defined on the global object before someone want the object. Is will not be much simpler than the current workaround (calling GetConstructorObject() before wrapping the object). It will even moot the lazy creation of interface objects.

I don't understand why this needs to be so complicated. AFAICT both for XULElement and for PaintRequest we just need to check IsChromeOrXBL to know whether to define them on the global or not. Why can't we, from inside GetProtoObject or GetConstructorObject, check for that and pass the result to CreateInterfaceObjects? Neither GetProtoObject or GetConstructorObject would need to take aDefineOnGlobal, they just need to pass the right value to CreateInterfaceObjects. And I'm saying to get the right value by adding a way to query the native type for it (eg with an optional NativeType::DefineInterfaceOnGlobal()). What am I missing?

(In reply to Peter Van der Beken [:peterv] from comment #23) > I don't understand why this needs to be so complicated. AFAICT both for > XULElement and for PaintRequest we just need to check IsChromeOrXBL to know > whether to define them on the global or not. Why can't we, from inside > GetProtoObject or GetConstructorObject, check for that and pass the result > to CreateInterfaceObjects? Neither GetProtoObject or GetConstructorObject > would need to take aDefineOnGlobal, they just need to pass the right value > to CreateInterfaceObjects. And I'm saying to get the right value by adding a > way to query the native type for it (eg with an optional > NativeType::DefineInterfaceOnGlobal()). What am I missing? I guess it will not work well with XRay. https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp?rev=28e8a3bbe870&mark=3042-3043,3058-3059,3065-3066#3035 Asking Bobby for confirmation.

Actually, let's ask bz what he thinks. It seems to me like we could just use ConstructorEnabled to decide whether to define inside GetConstructorObject and get exactly the same result as https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp?rev=28e8a3bbe870&mark=3058-3059,3065-3066#3035

Hmm. So get rid of the boolean argument in GetConstructorObject and instead codegen the right check there? It seems to me like that should work ok, actually. We'd need to keep doing the separate enabled check on the xray itself in the classinfo code linked to above, but that should be fine.

(In reply to Boris Zbarsky [:bz] from comment #26) > Hmm. So get rid of the boolean argument in GetConstructorObject and instead > codegen the right check there? > > It seems to me like that should work ok, actually. We'd need to keep doing > the separate enabled check on the xray itself in the classinfo code linked > to above, but that should be fine. Looks like GlobalResolve has to know whether the interface object was defined on the global or not. Otherwise did_resolve will not be set correctly. Then we will have to add an output parameter (e.g. aGlobalDefined) instead.

- Before the patch, sometimes pref changes were reflected to the global object without creating a new global object. - I have to change ChromeWorkerPrivate::WorkerAvailable() because it will be called from WorkerPrivate::RegisterBindings() before the worker script start running once this bug is fixed.

Rebased attachment 807504 [details] [diff] [review].

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0c133fd40b5a

(In reply to Masatoshi Kimura [:emk] from comment #28) > - Before the patch, sometimes pref changes were reflected to the global > object without creating a new global object. Could you explain what has changed here? I don't object to the changes, but I don't completely understand why they're needed now.

(In reply to Peter Van der Beken [:peterv] from comment #32) > Could you explain what has changed here? I don't object to the changes, but > I don't completely understand why they're needed now. Currently GlobalResolve will call DefineDOMInterface only if the pref is turned on. Therefore, if the interface object is not yet defined and pref is flipped on, the interface object will be created and defined without creating a new global object. https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp?rev=3b3ff422444d&mark=3031-3033#3026 I had to remove this because the new code no longer checks the availability separately. But the old code was incomplete anyway. If the interface object is already defined and the pref is flipped off, the interface object will be still present on the existing global object. This patch will make the behavior more consistent.

I tried understanding the explanation, but failed and ended up having to apply the patch to understand the difference. We currently don't call CreateInterfaceObjects at all when the pref is not set. With the patch, if the pref is not set we'll call CreateInterfaceObjects, create and store the interface object but not define it on the global. When the pref is flipped on, in the old code we'd call CreateInterfaceObjects, create and store the interface object and define it on the global. In the new code we'd not call CreateInterfaceObjects since we already have an interface object so we don't define it.

It's a little unfortunate that resolving a pref-disabled name would still create the interface object I guess :-/.

Is it better to introduce a bitmap again (as the first patch did) to avoid creating the interface object?

This keeps the behaviour much closer to what we have now, and doesn't require any test fixes. Still need to check whether the worker change is correct, but with that it's green on try.