Closed Bug 831367 Opened 7 years ago Closed 7 years ago

We should simplify SpecialPowersAPI.bindDOMWindowUtils()

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

RESOLVED FIXED
mozilla22
Tracking Status
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(2 files, 1 obsolete file)

The current implementation of SpecialPowersAPI.bindDomWindowUtils() was made before 'wrap' was available, and is a little needlessly complicated.  It also causes a memory leak when Marionette uses it, since Marionette instantiates a new SpecialPowers object with each execute_script call.

When an attempt was made to replace the current code with:

 function bindDOMWindowUtils(aWindow) {
   if (!aWindow)
     return
 
   var util = aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
   return wrapPrivileged(util);
 }

it broke a bunch of tests, which apparently rely on the current method of binding.  See bug 825802.  The affected tests are:

2642 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/contacts/tests/test_contacts_blobs.html | Got an array object - got false, expected true
2643 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/contacts/tests/test_contacts_blobs.html | uncaught exception - TypeError: blobs1 is null at http://mochi.test:8888/tests/dom/contacts/tests/test_contacts_blobs.html:147
2947 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_append_read_data.html | Correct blob data
2948 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_append_read_data.html | Correct size - got 118381, expected 218368
3452 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct location - got 118381, expected 218368
3453 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct location - got 118381, expected 218368
3454 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct blob data
3455 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct size - got 118381, expected 218368
19166 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_array.html | uncaught exception - DataCloneError: The object could not be cloned. at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_array.html:51
19169 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_cross_database_copying.html | indexedDB error, 'AbortError'
19170 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_cross_database_copying.html | uncaught exception - AbortError at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_array.html:37
19171 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_cross_database_copying.html | [SimpleTest.finish()] this test already called finish!
19172 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_cross_database_copying.html | [SimpleTest.finish()] this test already called finish!
19173 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_cross_database_copying.html | /tests/dom/indexedDB/test/test_file_array.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19176 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_delete.html | /tests/dom/indexedDB/test/test_file_array.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19181 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_os_delete.html | uncaught exception - DataCloneError: The object could not be cloned. at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_os_delete.html:40
19184 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_put_get_object.html | indexedDB error, 'AbortError'
19185 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_put_get_object.html | uncaught exception - AbortError at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_os_delete.html:27
19186 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_put_get_object.html | [SimpleTest.finish()] this test already called finish!
19187 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_put_get_object.html | [SimpleTest.finish()] this test already called finish!
19188 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_put_get_object.html | /tests/dom/indexedDB/test/test_file_os_delete.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19191 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_put_get_values.html | /tests/dom/indexedDB/test/test_file_os_delete.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19197 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_quota.html | uncaught exception - DataCloneError: The object could not be cloned. at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_quota.html:42
19301 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_replace.html | uncaught exception - DataCloneError: The object could not be cloned. at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_replace.html:39
19304 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_resurrection_delete.html | indexedDB error, 'AbortError'
19305 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_resurrection_delete.html | uncaught exception - AbortError at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_replace.html:25
19306 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_resurrection_delete.html | [SimpleTest.finish()] this test already called finish!
19307 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_resurrection_delete.html | [SimpleTest.finish()] this test already called finish!
19308 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_resurrection_delete.html | /tests/dom/indexedDB/test/test_file_replace.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19311 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_resurrection_transaction_abort.html | /tests/dom/indexedDB/test/test_file_replace.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19316 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_sharing.html | uncaught exception - DataCloneError: The object could not be cloned. at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_sharing.html:39
19319 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_transaction_abort.html | indexedDB error, 'AbortError'
19320 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_transaction_abort.html | uncaught exception - AbortError at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_sharing.html:26
19321 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_transaction_abort.html | [SimpleTest.finish()] this test already called finish!
19322 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_file_transaction_abort.html | [SimpleTest.finish()] this test already called finish!
19325 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_filehandle_quota.html | /tests/dom/indexedDB/test/test_file_transaction_abort.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
19352 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_filehandle_store_snapshot.html | Instance of nsIDOMFile - got true, expected false
19353 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_filehandle_store_snapshot.html | Correct size - got 13, expected 100000
19356 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_filehandle_store_snapshot.html | uncaught exception - NS_ERROR_FAILURE: Failure arg 0 [nsIDOMFileReader.readAsArrayBuffer] at http://mochi.test:8888/tests/dom/indexedDB/test/file.js:111

from https://tbpl.mozilla.org/php/getParsedLog.php?id=18831926&tree=Try&full=1

and

13341 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 0 for data [0, 100] - got [object HTMLDivElement], expected [object HTMLDivElement]
13342 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 1 for data [9, 109] - got [object HTMLDivElement], expected [object HTMLDivElement]
13345 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 4 for data [0, 100, true, false] - got [object HTMLDivElement], expected [object HTMLDivElement]
13346 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 5 for data [9, 109, true, false] - got [object HTMLDivElement], expected [object HTMLDivElement]
13347 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 6 for data [0, 100000, true, false] - got [object HTMLDivElement], expected [object HTMLDivElement]
13348 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 7 for data [9, 100009, true, false] - got [object HTMLDivElement], expected [object HTMLDivElement]
13349 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 8 for data [10, 110, false, true] - got [object HTMLDivElement], expected [object HTMLDivElement]
13350 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 9 for data [0, 100, false, false] - got [object HTMLHtmlElement], expected [object HTMLHtmlElement]
13351 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 10 for data [0, 100, false, true] - got [object HTMLDivElement], expected [object HTMLDivElement]
13352 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 11 for data [10, 100010, true, true] - got [object HTMLDivElement], expected [object HTMLDivElement]
13353 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 12 for data [0, 100000, true, false] - got [object HTMLHtmlElement], expected [object HTMLHtmlElement]
13354 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils.html | at index 13 for data [0, 100000, true, true] - got [object HTMLDivElement], expected [object HTMLDivElement]

from https://tbpl.mozilla.org/php/getParsedLog.php?id=18831873&tree=Try&full=1.

In order to change the way we bind DOMWindowUtils, we'll probably need to look at all of those tests and update them.
Bobby, I assume this is just wrapper trouble. Do you have any thoughts?
These look mostly like identity and instanceof issues. They can probably be fixed mechanically by unwrapping before comparing and doing SpecialPowers_instanceOf or whatever it's called.
Blocks: 838786
One of the things that fails with this patch is here:

http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/file.js#52

The Blob objects returned by this, after applying this patch, do not play nicely with indexedDB calls; it results in e.g., "failed | uncaught exception - DataCloneError: The object could not be cloned. at http://mochi.test:8888/tests/dom/indexedDB/test/test_file_array.html:52"

What do I need to unwrap to make this work?
See comment #3.
Flags: needinfo?(bobbyholley+bmo)
It looks like |utils| is a special-powers wrapped object, so anything returned by methods invoked on it will also be wrapped via the transitive behavior. If callers expect regular blobs, getBlob should probably SpecialPowers.unwrap before returning. Note however that, depending on the details of the underlying object, this may or may not return an object that's opaque to the associated content.
Flags: needinfo?(bobbyholley+bmo)
This works on my own machine, we'll see what try says: https://tbpl.mozilla.org/?tree=Try&rev=d4bfa1e985f6
Assignee: nobody → jgriffin
Try failures:

2689 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/contacts/tests/test_contacts_blobs.html | Got an array object - got false, expected true
2690 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/contacts/tests/test_contacts_blobs.html | uncaught exception - TypeError: blobs1 is null at http://mochi.test:8888/tests/dom/contacts/tests/test_contacts_blobs.html:147
3002 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_append_read_data.html | Correct blob data
3003 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_append_read_data.html | Correct size - got 118381, expected 218368
3507 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct location - got 118381, expected 218368
3508 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct location - got 118381, expected 218368
3509 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct blob data
3510 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/file/test/test_write_read_data.html | Correct size - got 118381, expected 218368

and

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js | application timed out after 330 seconds with no output
Fixed a few more cases
Attachment #714017 - Attachment is obsolete: true
The browser-chrome crash/hang has occurred over a couple of different try runs.  See e.g., 

https://tbpl.mozilla.org/php/getParsedLog.php?id=19747713&tree=Try&full=1 - PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js | application crashed [@ libc-2.11.so + 0xd4aa3]

and 

https://tbpl.mozilla.org/php/getParsedLog.php?id=19747803&tree=Try&full=1 -
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js | application timed out after 330 seconds with no output

The debugger tests don't use DOMWindowUtils at all, and only reference SpecialPowers in a couple of places in this file:  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_propertyview-09.js

Any ideas what might be going on, ted or bholley?
Note that we intentionally kill the process when we detect a hang, so the crash is just whatever the browser was doing when we killed it. Here it just looks like it's spinning the event loop. It looks like that test hooks up the Debugger API, creates a new iframe, and expects to get a "newScript" notification for its global, which it never receives, and thus hangs:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js

past, jimb: any idea why the patch here (to SpecialPowers) would break this test in this way?
This test takes a while to run. At the point where it times out, it should be adding all globals in the browser instance as debuggees, which takes a few seconds even in my i7. Perhaps this change makes the test run a little slower, which tips it off the standard threshold in our slow machines on try. Can you see if requesting a longer timeout fixes it?
(In reply to Panos Astithas [:past] from comment #13)
> which tips it off the standard threshold in our slow machines on try.

s/off/over/
(In reply to Panos Astithas [:past] from comment #14)
> (In reply to Panos Astithas [:past] from comment #13)
> > which tips it off the standard threshold in our slow machines on try.
> 
> s/off/over/

The current timeout is ~300s; should it take that long to run?
(In reply to Jonathan Griffin (:jgriffin) from comment #15)
> (In reply to Panos Astithas [:past] from comment #14)
> > (In reply to Panos Astithas [:past] from comment #13)
> > > which tips it off the standard threshold in our slow machines on try.
> > 
> > s/off/over/
> 
> The current timeout is ~300s; should it take that long to run?

I don't think so, but I'm not sure. Could you do another try push with the patch from bug 758172 to get some more logging?
I added that to the patch and pushed to try again:  https://tbpl.mozilla.org/?tree=Try&rev=c7cc07325fed
The problem seems to be that SpiderMonkey does not generate a "newScript" event in time, after the "newGlobal" event we see in the log prior to the timeout. Other tests properly receive such events, so it doesn't look like a more general issue.

My best guess is still that wrapPrivileged is a bit slower than the current code and requesting a longer timeout might fix this already slow test. Can we try that?
Alternatively, here is a change in the test that might make it quicker to finish, by ensuring the expected scripts are generated faster.
I have an awfully hard time believing that wrapPrivileged makes this test take >5 minutes to complete. I would guess there's a more subtle bug here, and perhaps the existing SpecialPowers code makes things work, but changing it makes them break.
Pushed with past's patch to try:  https://tbpl.mozilla.org/?tree=Try&rev=719d99c565cc
(In reply to Jonathan Griffin (:jgriffin) from comment #21)
> Pushed with past's patch to try: 
> https://tbpl.mozilla.org/?tree=Try&rev=719d99c565cc

This works...browser-chrome is all green!  Thanks past.  Is there any reason not to fold your patch into this one?
Flags: needinfo?(past)
(In reply to Jonathan Griffin (:jgriffin) from comment #22)
> This works...browser-chrome is all green!  Thanks past.  Is there any reason
> not to fold your patch into this one?

No, it's actually better than before. Feel free to land them both.
Flags: needinfo?(past)
Running this one more on try, against all the things:  https://tbpl.mozilla.org/?tree=Try&rev=e825dda465fb
There were some oranges here on debug Windows, I'm doing some retriggers to see if they go away.
Attachment #714164 - Flags: review?(ted)
Attachment #715384 - Flags: review?(ted)
try is green
Attachment #714164 - Flags: review?(ted) → review+
Comment on attachment 715384 [details] [diff] [review]
Fix for chrome-debugging test

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

This should probably get a review from a peer of this code, but this is past's patch so if he's ok with me r+ing it then I'm ok.
Attachment #715384 - Flags: review?(ted) → review+
Given past's comment #23, I think we're good to land.
https://hg.mozilla.org/mozilla-central/rev/a2dbc433130f
https://hg.mozilla.org/mozilla-central/rev/86a5cbda14e9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.