Closed Bug 1251855 Opened 4 years ago Closed 4 years ago

test_xrayToJS.xul is going to permafail when Gecko 47 merges to Beta

Categories

(Core :: XPConnect, defect, major)

defect
Not set
major

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)

[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
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?
Er, actually needinfoing Till too.
Flags: needinfo?(till)
Is AppConstants.RELEASE_BUILD usable in this test?
(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)
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)
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
> 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").
Try confirms this works as expected for both Trunk and Trunk-as-Beta.
Attachment #8727141 - Flags: review?(bzbarsky)
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+
https://hg.mozilla.org/mozilla-central/rev/73c0af3f3693
Status: NEW → RESOLVED
Closed: 4 years ago
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)
Yes it does, and I'm planning to do so still :)
Flags: needinfo?(ryanvm)
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 → ---
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)
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.
In particular, that bug landed right before 47 branched to Aurora, afaict...
Flags: needinfo?(till)
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: 4 years ago4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.