Closed Bug 774775 Opened 8 years ago Closed 8 years ago

XRayWrapper does not handle new bindings constants

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox14 --- affected
firefox15 + fixed
firefox16 + fixed
firefox17 + fixed

People

(Reporter: khuey, Assigned: peterv)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

XRayWrapper doesn't pass through constants properly on new bindings.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 774152
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 774152
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: REOPENED → ASSIGNED
Attached patch v1.1Splinter Review
Attachment #643134 - Attachment is obsolete: true
Attachment #643151 - Flags: review?(bzbarsky)
Blocks: 773460
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...
Attachment #643151 - Flags: review?(bzbarsky) → review+
Attached patch Refactoring v1.1Splinter Review
Attachment #643209 - Attachment is obsolete: true
Attachment #643277 - Flags: review?(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.  ;)
Attachment #643277 - Flags: review?(bzbarsky) → review+
Tracking for 15 and up, but I'm curious what the user impact to our 14 users is.
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.
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.
(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?
This bug affects the Azure canvas on Windows.  It only 'breaks' accessing constants on a content object from chrome through.
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.
And yes, as comment 14 says the scope of the problem is pretty limited.
(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.
> Azure/Skia canvas is not meant to be on anywhere by default

It's not, because Skia is not on by default.
https://hg.mozilla.org/mozilla-central/rev/03a0d026fca1
https://hg.mozilla.org/mozilla-central/rev/37898d7d1189
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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
Attachment #643151 - Flags: approval-mozilla-aurora?
Comment on attachment 643277 [details] [diff] [review]
Refactoring v1.1

See comment 20
Attachment #643277 - Flags: approval-mozilla-aurora?
Does anyone think we should take this on beta?
I think it's probably a good idea.  We did the WebGL context in 15, and that has a bunch of constants on it....
Attachment #643151 - Flags: approval-mozilla-beta?
Attachment #643277 - Flags: approval-mozilla-beta?
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.
Attachment #643151 - Flags: approval-mozilla-beta?
Attachment #643151 - Flags: approval-mozilla-beta+
Attachment #643151 - Flags: approval-mozilla-aurora?
Attachment #643151 - Flags: approval-mozilla-aurora+
Attachment #643277 - Flags: approval-mozilla-beta?
Attachment #643277 - Flags: approval-mozilla-beta+
Attachment #643277 - Flags: approval-mozilla-aurora?
Attachment #643277 - Flags: approval-mozilla-aurora+
Er... Peter, does that test fail without this patch?  I don't see why this sandbox would be using Xrays, exactly...
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.
Marking as [qa-] since there is a test verifying this bug fix in the push (https://hg.mozilla.org/releases/mozilla-beta/rev/984e5eee5eea).
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.