918567 - Hide interface objects for MozAfterPaint from content

Status: ASSIGNED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: hide_afterpaint

MozAfterPaint event is unavailable from content by default.

Comment 1

[Masatoshi Kimura [:emk]]

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

Comment 2

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6b5cb3fe3b

Comment 3

[Ed Morley [:emorley]]

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

Comment 4

[Ed Morley [:emorley]]

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 :-)

Comment 5

[Ed Morley [:emorley]]

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

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Fixed orange on Windows and B2G.

Comment 7

[Masatoshi Kimura [:emk]]

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

Comment 8

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

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?

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: patch v3

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

Comment 10

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

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

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Introduce a struct contaning protoIfaceArray

Rebased patches of bug 909340.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Decouple defining interface objects from the creation

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Stop defining interface objects in GetProtoObject

Comment 14

[Masatoshi Kimura [:emk]]

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

Comment 15

[Masatoshi Kimura [:emk]]

Ping?

Comment 16

[Peter Van der Beken [:peterv]]

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.

Comment 17

[Masatoshi Kimura [:emk]]

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.

Comment 18

[Masatoshi Kimura [:emk]]

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.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: XULElementBinding.cpp before patching

For ease of review.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: XULElementBinding.cpp after the patch

Comment 21

[Peter Van der Beken [:peterv]]

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)?

Comment 22

[Masatoshi Kimura [:emk]]

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.

Comment 23

[Peter Van der Beken [:peterv]]

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?

Comment 24

[Masatoshi Kimura [:emk]]

(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.

Comment 25

[Peter Van der Beken [:peterv]]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Masatoshi Kimura [:emk]]

(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.

Comment 28

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Test fixup

- 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.

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Call ConstructorEnabled() inside CreateInterfaceObjects() to determine whether th interfaceobject should be defined on the global object or not

Comment 30

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Hide interface objects for MozAfterPaint from content. r=smaug

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

Comment 31

[Masatoshi Kimura [:emk]]

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

Comment 32

[Peter Van der Beken [:peterv]]

(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.

Comment 33

[Masatoshi Kimura [:emk]]

(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.

Comment 34

[Peter Van der Beken [:peterv]]

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.

Comment 35

[Peter Van der Beken [:peterv]]

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

Comment 36

[Masatoshi Kimura [:emk]]

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

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: Alternate approach v1

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.