Last Comment Bug 774775 - XRayWrapper does not handle new bindings constants
: XRayWrapper does not handle new bindings constants
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
: 774152 (view as bug list)
Depends on:
Blocks: 773460
  Show dependency treegraph
 
Reported: 2012-07-17 11:32 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2012-11-15 02:23 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
+
fixed


Attachments
v1 (3.52 KB, patch)
2012-07-17 13:57 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v1.1 (6.31 KB, patch)
2012-07-17 14:55 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Refactoring I did on the way to maybe fixing this bug, in case we ever want it (12.65 KB, patch)
2012-07-17 17:56 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Refactoring v1.1 (16.36 KB, patch)
2012-07-18 00:23 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-17 11:32:42 PDT
XRayWrapper doesn't pass through constants properly on new bindings.
Comment 1 Josh Matthews [:jdm] 2012-07-17 11:37:14 PDT

*** This bug has been marked as a duplicate of bug 774152 ***
Comment 2 Josh Matthews [:jdm] 2012-07-17 11:37:58 PDT
*** Bug 774152 has been marked as a duplicate of this bug. ***
Comment 3 Peter Van der Beken [:peterv] 2012-07-17 13:57:52 PDT
Created attachment 643134 [details] [diff] [review]
v1
Comment 4 Peter Van der Beken [:peterv] 2012-07-17 14:55:43 PDT
Created attachment 643151 [details] [diff] [review]
v1.1
Comment 5 Boris Zbarsky [:bz] 2012-07-17 17:56:50 PDT
Created attachment 643209 [details] [diff] [review]
Refactoring I did on the way to maybe fixing this bug, in case we ever want it
Comment 6 Boris Zbarsky [:bz] 2012-07-17 18:04:48 PDT
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] 2012-07-18 00:23:55 PDT
Created attachment 643277 [details] [diff] [review]
Refactoring v1.1
Comment 8 Boris Zbarsky [:bz] 2012-07-18 01:16:39 PDT
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 10 Alex Keybl [:akeybl] 2012-07-18 17:05:41 PDT
Tracking for 15 and up, but I'm curious what the user impact to our 14 users is.
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-18 17:15:08 PDT
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 (:kentuckyfriedtakahe, :k17e) 2012-07-18 17:32:19 PDT
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] 2012-07-18 19:41:25 PDT
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-18 19:43:22 PDT
This bug affects the Azure canvas on Windows.  It only 'breaks' accessing constants on a content object from chrome through.
Comment 15 Boris Zbarsky [:bz] 2012-07-18 19:45:22 PDT
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 [:bz] 2012-07-18 19:46:04 PDT
And yes, as comment 14 says the scope of the problem is pretty limited.
Comment 17 Nick Cameron [:nrc] 2012-07-18 19:57:39 PDT
(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 [:bz] 2012-07-18 20:15:05 PDT
> Azure/Skia canvas is not meant to be on anywhere by default

It's not, because Skia is not on by default.
Comment 20 Peter Van der Beken [:peterv] 2012-07-20 00:17:59 PDT
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] 2012-07-20 00:18:28 PDT
Comment on attachment 643277 [details] [diff] [review]
Refactoring v1.1

See comment 20
Comment 22 Peter Van der Beken [:peterv] 2012-07-20 00:18:55 PDT
Does anyone think we should take this on beta?
Comment 23 Boris Zbarsky [:bz] 2012-07-20 00:21:12 PDT
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] 2012-07-20 15:27:08 PDT
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 [:bz] 2012-07-27 13:47:14 PDT
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 [:bz] 2012-07-30 13:21:25 PDT
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 28 Ioana (away) 2012-11-15 02:23:40 PST
Marking as [qa-] since there is a test verifying this bug fix in the push (https://hg.mozilla.org/releases/mozilla-beta/rev/984e5eee5eea).

Note You need to log in before you can comment on or make changes to this bug.