Closed
Bug 1251855
Opened 8 years ago
Closed 8 years ago
test_xrayToJS.xul is going to permafail when Gecko 47 merges to Beta
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
WORKSFORME
mozilla48
Tracking | Status | |
---|---|---|
firefox45 | --- | unaffected |
firefox46 | --- | unaffected |
firefox47 | - | unaffected |
firefox48 | --- | unaffected |
People
(Reporter: RyanVM, Assigned: till)
Details
(Whiteboard: btpp-active btpp-follow-up-2016-04-15)
Attachments
(1 file)
3.74 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]: Merge day permafail. Looks like "entries" and "values" are in the expected list, but not the actual. https://treeherder.mozilla.org/logviewer.html#?job_id=17314020&repo=try 21:28:32 INFO - 2743 INFO TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xul | A property on the Object constructor has changed! You need a security audit from an XPConnect peer - got "[\"assign\", \"create\", \"defineProperties\", \"defineProperty\", \"freeze\", \"getOwnPropertyDescriptor\", \"getOwnPropertyNames\", \"getOwnPropertySymbols\", \"getPrototypeOf\", \"is\", \"isExtensible\", \"isFrozen\", \"isSealed\", \"keys\", \"length\", \"name\", \"preventExtensions\", \"prototype\", \"seal\", \"setPrototypeOf\"]", expected "[\"assign\", \"create\", \"defineProperties\", \"defineProperty\", \"entries\", \"freeze\", \"getOwnPropertyDescriptor\", \"getOwnPropertyNames\", \"getOwnPropertySymbols\", \"getPrototypeOf\", \"is\", \"isExtensible\", \"isFrozen\", \"isSealed\", \"keys\", \"length\", \"name\", \"preventExtensions\", \"prototype\", \"seal\", \"setPrototypeOf\", \"values\"]" 21:28:32 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:267:5 21:28:32 INFO - testXray@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:387:5 21:28:32 INFO - testObject@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:425:5 21:28:32 INFO - go@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:124:5 21:28:32 INFO - onload@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:1:1
Comment 1•8 years ago
|
||
From js/src/builtin/Object.cpp: 1010 #ifndef RELEASE_BUILD 1011 JS_SELF_HOSTED_FN("values", "ObjectValues", 1, JSPROP_DEFINE_LATE), 1012 JS_SELF_HOSTED_FN("entries", "ObjectEntries", 1, JSPROP_DEFINE_LATE), 1013 #endif We could probably do version sniffing and set the expected array accordingly. Or we could just patch this on beta, if the expectation is that this will get enabled on RELEASE_BUILD soon. Till, do you know what the plan there is? Bug 1232639 seems pretty stalled out, so maybe we should just sniff for now... Ryan, do you know offhand whether we have an approved method of channel detection in tests?
Reporter | ||
Comment 3•8 years ago
|
||
Is AppConstants.RELEASE_BUILD usable in this test?
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1) > We could probably do version sniffing and set the expected array > accordingly. Or we could just patch this on beta, if the expectation is > that this will get enabled on RELEASE_BUILD soon. Till, do you know what > the plan there is? Bug 1232639 seems pretty stalled out, so maybe we should > just sniff for now... Bleh, let's patch this on beta. This really should ride the trains soon, and there isn't any truly fundamental reason not to land bug 1232639 with little tweaks. I'll prepare patches for both (this bug and bug 1232639) early next week. Ryan, can I attach the patch for beta here and coordinate with sheriffs for landing it on merge day?
Flags: needinfo?(till) → needinfo?(ryanvm)
Reporter | ||
Comment 5•8 years ago
|
||
Assuming it's not going to be a regular occurrence, shouldn't be a big deal. Note that this is for the April merge day.
Flags: needinfo?(ryanvm)
Comment 6•8 years ago
|
||
Giving to Till based on comment 4. I'll followup just before the merge.
Assignee: nobody → till
Whiteboard: btpp-active btpp-follow-up-2016-04-15
Comment 7•8 years ago
|
||
> Is AppConstants.RELEASE_BUILD usable in this test?
For the record, I tried this, and the answer is yes, if the test first does Cu.import("resource://gre/modules/AppConstants.jsm").
Reporter | ||
Comment 8•8 years ago
|
||
Try confirms this works as expected for both Trunk and Trunk-as-Beta.
Attachment #8727141 -
Flags: review?(bzbarsky)
Comment 9•8 years ago
|
||
Comment on attachment 8727141 [details] [diff] [review] make use of AppConstants.RELEASE_BUILD Let's not have all that duplication. In other words, do it like so: gConstructorProperties['Object'] = constructorProps([stuff]); if (!AppConstants.RELEASE_BUILD) { gConstructorProperties['Object'] = gConstructorProperties['Object'].concat(["values", "entries"]); } or so. r=me with that.
Attachment #8727141 -
Flags: review?(bzbarsky) → review+
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73c0af3f3693
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Given that this is already fixed, I don't feel the need to track this. Ryan, does this need to be uplifted to Aurora47? Just wondering as it landed on Nightly48.
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 13•8 years ago
|
||
Yes it does, and I'm planning to do so still :)
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/b44401afec64
Reporter | ||
Comment 15•8 years ago
|
||
hmm, this appears to still be failing :( https://treeherder.mozilla.org/logviewer.html#?job_id=18728691&repo=try 23:54:43 INFO - 2744 INFO TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xul | A property on the Object constructor has changed! You need a security audit from an XPConnect peer - got "[\"assign\", \"create\", \"defineProperties\", \"defineProperty\", \"entries\", \"freeze\", \"getOwnPropertyDescriptor\", \"getOwnPropertyNames\", \"getOwnPropertySymbols\", \"getPrototypeOf\", \"is\", \"isExtensible\", \"isFrozen\", \"isSealed\", \"keys\", \"length\", \"name\", \"preventExtensions\", \"prototype\", \"seal\", \"setPrototypeOf\", \"values\"]", expected "[\"assign\", \"create\", \"defineProperties\", \"defineProperty\", \"freeze\", \"getOwnPropertyDescriptor\", \"getOwnPropertyNames\", \"getOwnPropertySymbols\", \"getPrototypeOf\", \"is\", \"isExtensible\", \"isFrozen\", \"isSealed\", \"keys\", \"length\", \"name\", \"preventExtensions\", \"prototype\", \"seal\", \"setPrototypeOf\"]"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 16•8 years ago
|
||
Looks like it's failing in the opposite way now. Till, did you land & uplift patches that rendered the original patch obsolete?
Flags: needinfo?(till)
Comment 17•8 years ago
|
||
Hmm. So that's the other way around, right? AppConstants.RELEASE_BUILD was true, so we didn't add "values" and "entries" to the expected list. But the actual property is present on the object. And it's present because bug 1232639 was fixed for Firefox 47 (after the patch was posted in this bug, but before it was checked in) and unconditionally enabled the properties. So they're going to be enabled in 47 as far as I can tell, and we should actually back the patch for this bug out.
Comment 18•8 years ago
|
||
In particular, that bug landed right before 47 branched to Aurora, afaict...
Flags: needinfo?(till)
Reporter | ||
Comment 19•8 years ago
|
||
Thanks for confirming. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/fceee99ec164 https://hg.mozilla.org/releases/mozilla-aurora/rev/b5878f102851
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•