774775 - XRayWrapper does not handle new bindings constants

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

XRayWrapper doesn't pass through constants properly on new bindings.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Refactoring I did on the way to maybe fixing this bug, in case we ever want it

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 643151 [details] [diff] [review]
v1.1

s/constanst/constants/ and s/Otherwis/Otherwise/

r=me, though maybe we should consider reducing codesize by refactoring this into a function like in the diff I attached...

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Refactoring v1.1

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 643277 [details] [diff] [review]
Refactoring v1.1

r=me.  I take it you were ok with the parts of the code I'd already written.  ;)

Comment 9

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03a0d026fca1
https://hg.mozilla.org/integration/mozilla-inbound/rev/37898d7d1189

Comment 10

[Alex Keybl [:akeybl]]

Tracking for 15 and up, but I'm curious what the user impact to our 14 users is.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This might break addons, but given that we only converted xhr its unlikely.  It's worse now on trunk because we converted canvas on certain platforms.  That broke e.g. out of process reftest and is much more likely to break addons.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

It only broke Azure Canvas which is prefed off by default as it is still in development. This bug is blocking turning Azure Canvas on.

Comment 13

[Nick Cameron [:nrc]]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #12)
> It only broke Azure Canvas which is prefed off by default as it is still in
> development. This bug is blocking turning Azure Canvas on.

To clarify, it breaks Azure/Cairo canvas which has not yet landed. Azure/D2D canvas is on by default for Windows, I am not sure if this breaks it or how thoroughly it is being tested - Bas?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This bug affects the Azure canvas on Windows.  It only 'breaks' accessing constants on a content object from chrome through.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That bug you mention is for the Azure-cairo canvas.

If you actually look at the gfx.canvas.azure.enabled pref, it defaults to true right now, so you get Azure canvas if AzureCanvasEnabledOnPlatform is true.  Which it is on Linux, Android Mac, and on Windows if either direct2d or skia is being used as the rendering backend.

So basically, if you're not on WinXP you're using Azure canvas today.

Comment 16

[Boris Zbarsky [:bzbarsky]]

And yes, as comment 14 says the scope of the problem is pretty limited.

Comment 17

[Nick Cameron [:nrc]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> That bug you mention is for the Azure-cairo canvas.
> 
> If you actually look at the gfx.canvas.azure.enabled pref, it defaults to
> true right now, so you get Azure canvas if AzureCanvasEnabledOnPlatform is
> true.  Which it is on Linux, Android Mac, and on Windows if either direct2d
> or skia is being used as the rendering backend.
> 
> So basically, if you're not on WinXP you're using Azure canvas today.

Hmm, that is a bit scary, Azure/Skia canvas is not meant to be on anywhere by default :-s I'll look into it, but I guess this bug is not the right place for discussing it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Azure/Skia canvas is not meant to be on anywhere by default

It's not, because Skia is not on by default.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/03a0d026fca1
https://hg.mozilla.org/mozilla-central/rev/37898d7d1189

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 643151 [details] [diff] [review]
v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 740069
User impact if declined: could break addons (we didn't get any reports yet)
Testing completed (on m-c, etc.): landed on mozilla-central, has a test
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 643277 [details] [diff] [review]
Refactoring v1.1

See comment 20

Comment 22

[Peter Van der Beken [:peterv]]

Does anyone think we should take this on beta?

Comment 23

[Boris Zbarsky [:bzbarsky]]

I think it's probably a good idea.  We did the WebGL context in 15, and that has a bunch of constants on it....

Comment 24

[Alex Keybl [:akeybl]]

Comment on attachment 643151 [details] [diff] [review]
v1.1

[Triage Comment]
Low risk fix that will keep us from scratching our heads when users report new add-on regressions. Approved for Aurora 16 and Beta 15.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Er... Peter, does that test fail without this patch?  I don't see why this sandbox would be using Xrays, exactly...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Nevermind.  It defaults to Xrays because of the about:blank bits...  I'll just add a test to make sure it's really got Xrays.

Comment 27

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c98bb360201d
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc561d1aa5a6
https://hg.mozilla.org/releases/mozilla-beta/rev/984e5eee5eea
https://hg.mozilla.org/releases/mozilla-beta/rev/533ea5877be6

Comment 28

[Ioana (away)]

Marking as [qa-] since there is a test verifying this bug fix in the push (https://hg.mozilla.org/releases/mozilla-beta/rev/984e5eee5eea).