Closed Bug 1038647 Opened 10 years ago Closed 10 years ago

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

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird33 fixed, thunderbird34 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 34.0
Tracking Flags:
Tracking Status
thunderbird33 --- fixed
thunderbird34 --- fixed

People

(Reporter: mkmelin, Assigned: mconley)

References

Details

(Keywords: intermittent-failure)

Attachments

(4 obsolete files) 

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) ];
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)
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)
Sorry, I don't know.
Flags: needinfo?(mkmelin+mozilla)
(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)
Assignee: nobody → mconley
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)
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)
Attached patch Tests. v2 r=bholley (obsolete) (deleted) — — Splinter Review 

        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

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Waive Xray wrapper on custom iterator (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

    
    

    
  
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)

    
    

    
  
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 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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 34.0

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: