bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

perma orange: TEST-UNEXPECTED-FAIL | mozmill/utils/test-iteratorUtils.js | test-iteratorUtils.js::test_toArray_custom_content_iterator

RESOLVED FIXED in Thunderbird 34.0

Status

Thunderbird
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Magnus Melin, Assigned: mconley)

Tracking

({intermittent-failure})

Trunk
Thunderbird 34.0
intermittent-failure

Thunderbird Tracking Flags

(thunderbird33 fixed, thunderbird34 fixed)

Details

Attachments

(4 obsolete attachments)

(Reporter)

Description

4 years ago
Starting https://hg.mozilla.org/comm-central/rev/b64ae233cdde - but that hardly caused it, so something from m-c I guess.

https://tbpl.mozilla.org/php/getParsedLog.php?id=43778116&tree=Thunderbird-Trunk&full=1

SUMMARY-UNEXPECTED-FAIL | test-iteratorUtils.js | test-iteratorUtils.js::test_toArray_custom_content_iterator
  EXCEPTION: undefined is not a function
    at: iteratorUtils.jsm line 42
       toArray iteratorUtils.jsm:42 21
       test_toArray_custom_content_iterator test-iteratorUtils.js:78 19
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-iteratorUtils.js::teardownModule

--> http://mxr.mozilla.org/comm-central/source/mailnews/base/util/iteratorUtils.jsm#42
42     return [ a for (a in aObj) ];

Comment 1

4 years ago
There are some lines before the failure that may be relevant:
[3424] WARNING: NS_ENSURE_TRUE(currentURI) failed: file /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/content/base/src/ThirdPartyUtil.cpp, line 96
[3424] WARNING: Silently denied access to property |next|: Object is not safely Xrayable (@resource:///modules/iteratorUtils.jsm:42): file /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/js/xpconnect/wrappers/XrayWrapper.cpp, line 588

mconley, you seem to be the author of this test, can you please look at this?
Flags: needinfo?(mconley)
This sounds like it's related to the XRay stuff that bholley just landed in bug 856067 (which is, unfortunately, a security bug, so I can't read it just yet).

The test that's failing definitely sounds like it'd use XRays - https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/utils/test-iteratorUtils.js#77.

I think the simple solution is to waive the XRay wrapper of the iterator coming in: https://developer.mozilla.org/en-US/docs/Xray_vision#Waiving_Xray_vision

I'm not _entirely_ sure why we need or want to iterate custom iterators coming in from content though... looking back through bug, it looks like something sid0 requested that we test...

I'd actually be fine removing this test for content iterators entirely unless anybody can put forward an argument about why we'd actually want it.

sid0 - do you remember why we'd want it? Or Magnus?
Flags: needinfo?(sid.bugzilla)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(mconley)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
I talked to sid0 today in IRC, and he can't remember why he asked me for it. :/

Hey bholley, I was wondering if you could advise us here for a sec, since you are very much a master of Xray wrapping and manipulations across the content/chrome boundary.

The test that is failing attempts to take an object from content, and iterate through it. The object is not an Iterator, but defines an __iterator__ property on itself which yields values. I believe we still do support that rather antiquated style of defining an iterator.

Anyhow, when attempting to iterate that object with a for...in loop, Javascript complains that "EXCEPTION: undefined is not a function".

I originally attempted to fix this by waiving the Xrays on the item brought in from content, thinking that this would allow us to iterate it, but again, the same error came up.

Do you know what we might be running in here?

And before you mention, yes, I do think it's strange that we want to support iterating iterables defined with __iterator__ from content. It's not clear to me if that's a thing we still want to do, because the original purpose of this test case is quite unclear. We might want to investigate removing support for that, but in the short term, if there's a workaround for this orange, we might want to start there.
Flags: needinfo?(sid.bugzilla) → needinfo?(bobbyholley)
(In reply to Mike Conley (:mconley) from comment #29)
> I talked to sid0 today in IRC, and he can't remember why he asked me for it.
> :/
> 
> Hey bholley, I was wondering if you could advise us here for a sec, since
> you are very much a master of Xray wrapping and manipulations across the
> content/chrome boundary.
> 
> The test that is failing attempts to take an object from content, and
> iterate through it. The object is not an Iterator, but defines an
> __iterator__ property on itself which yields values. I believe we still do
> support that rather antiquated style of defining an iterator.

Yes, but we certainly don't let you do that over Xrays. The content can define pretty arbitrary behavior for that, so you'd need to waive Xrays (Cu.waiveXrays) in order to see it.
Flags: needinfo?(bobbyholley)
I should have been more clear - even when we waive the Xrays, the "EXCEPTION: undefined is not a function" error is raised. Is iterating something like this with Xrays waived something that Spidermonkey just can't do?
Flags: needinfo?(bobbyholley)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 35

4 years ago
Sorry, I don't know.
Flags: needinfo?(mkmelin+mozilla)
Comment hidden (Treeherder Robot)
(In reply to Mike Conley (:mconley) from comment #31)
> I should have been more clear - even when we waive the Xrays, the
> "EXCEPTION: undefined is not a function" error is raised. Is iterating
> something like this with Xrays waived something that Spidermonkey just can't
> do?

If Xrays are waived, the behavior should be exactly the same as if code if one iframe manipulates objects in another same-origin iframe. If you can write an xpcshell-test that demonstrates otherwise, I'll take a look:

const Cu = Components.utils;
function run_test() {
  var sbchrome = Cu.Sandbox(this);
  var sbcontent1 = Cu.Sandbox('http://www.example.com');
  var sbcontent2 = Cu.Sandbox('http://www.example.com');
  var contentObj = Cu.evalInSandbox('...', sbcontent1);
  sbcontent2.contentObj = contentObj;
  sbchrome.contentObj = Cu.waiveXrays(contentObj);
  // Run equivalent coe in sbchrome and sbcontent2. If behavior differs, that's a bug.
}
Flags: needinfo?(bobbyholley)
(the xpcshell-test should live in js/xpconnect/tests/unit)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Assignee: nobody → mconley
Created attachment 8458420 [details] [diff] [review]
Add test case for iterating custom iterator from content with waived Xrays
Comment on attachment 8458420 [details] [diff] [review]
Add test case for iterating custom iterator from content with waived Xrays

So, I just kinda knocked this out, and it's not got the last layer of polish, but running it, I get:

From _tests: Kept 18535 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:03.78 INFO | Using at most 32 threads.
 0:03.87
 0:03.87 TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
 0:03.87
 0:03.87 TEST-UNEXPECTED-FAIL | /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js | TypeError: undefined is not a function at /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js:22 - See following stack:

JS frame :: /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js:22 :: run_test :: line 5
JS frame :: _execute_test@/Users/mikeconley/Projects/mozilla-central/testing/xpcshell/head.js:403:5
JS frame :: -e:1 :: anonymous :: line 1
 0:03.88 TEST-INFO | test_xrayed_iterator.js | Test failed or timed out, will retry.
 0:03.90 Retrying tests that failed when run in parallel.
 0:03.98
 0:03.98 TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
 0:03.98
 0:03.98 TEST-UNEXPECTED-FAIL | /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js | TypeError: undefined is not a function at /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js:22 - See following stack:

JS frame :: /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js:22 :: run_test :: line 5
JS frame :: _execute_test@/Users/mikeconley/Projects/mozilla-central/testing/xpcshell/head.js:403:5
JS frame :: -e:1 :: anonymous :: line 1
 0:03.99 TEST-UNEXPECTED-FAIL | /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/_tests/xpcshell/js/xpconnect/tests/unit/test_xrayed_iterator.js | test failed (with xpcshell return code: 0)
 0:03.99 >>>>>>>
 0:03.99 <<<<<<<
 0:03.99 INFO | Result summary:
 0:03.99 INFO | Passed: 0
 0:03.99 INFO | Failed: 1
 0:03.99 INFO | Todo: 0
 0:03.99 INFO | Retried: 1

So I _think_ I've got the workings of a test case here. Am I doing anything silly in here that might account for the error I'm getting, or have I found a real bug?
Attachment #8458420 - Flags: feedback?(bobbyholley)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Created attachment 8460631 [details] [diff] [review]
Add a WaiveXrayWrapper override for ::iterate. v1

I thought we had overrides for all the proxy traps that returned non-primitive
values, but it looks like we missed one.
Attachment #8460631 - Flags: review?(gkrizsanits)
Created attachment 8460632 [details] [diff] [review]
Tests. v2 r=bholley
Attachment #8458420 - Attachment is obsolete: true
Attachment #8458420 - Flags: feedback?(bobbyholley)
Attachment #8460632 - Flags: review+
Actually, I think this should go in a separate bug. I'll file.
Attachment #8460631 - Attachment is obsolete: true
Attachment #8460631 - Flags: review?(gkrizsanits)
Attachment #8460632 - Attachment is obsolete: true
Depends on: 1042398
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Created attachment 8460731 [details] [diff] [review]
Waive Xray wrapper on custom iterator
Comment on attachment 8460731 [details] [diff] [review]
Waive Xray wrapper on custom iterator

Hey Magnus, mind reviewing this one-liner? We'll need to wait for bug 1042398 to land on mozilla-central, but then this will take care of the permanent orange.
Attachment #8460731 - Flags: review?(mkmelin+mozilla)
(Reporter)

Comment 76

4 years ago
Comment on attachment 8460731 [details] [diff] [review]
Waive Xray wrapper on custom iterator

Review of attachment 8460731 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin
Attachment #8460731 - Flags: review?(mkmelin+mozilla) → review+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment on attachment 8460731 [details] [diff] [review]
Waive Xray wrapper on custom iterator

So I just found out we don't actually need to waive these Xrays in order for things to work properly. Once the tree re-opens and something lands, this should be fixed now that bug 1042398 has landed.
Attachment #8460731 - Attachment is obsolete: true
With bug 1042398 landed, this failure is gone.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 34.0
status-thunderbird33: --- → fixed
status-thunderbird34: --- → fixed
You need to log in before you can comment on or make changes to this bug.