Last Comment Bug 601277 - Write tests for document.domain
: Write tests for document.domain
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mounir Lamouri (:mounir)
Depends on: 729150
Blocks: CVE-2012-3985
  Show dependency treegraph
Reported: 2010-10-01 16:06 PDT by Mounir Lamouri (:mounir)
Modified: 2012-07-12 09:35 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Tests (7.79 KB, patch)
2010-10-11 03:36 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Tests (7.95 KB, patch)
2011-01-27 08:36 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Better tests. v1 (6.77 KB, patch)
2012-06-28 05:11 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2010-10-01 16:06:52 PDT

Comment 1 Mounir Lamouri (:mounir) 2010-10-11 03:36:28 PDT
Created attachment 482208 [details] [diff] [review]
Comment 2 Mounir Lamouri (:mounir) 2010-10-28 04:11:59 PDT
FWIW, these tests are no longer working.
Comment 3 Johnny Stenback (:jst, 2010-11-05 10:41:41 PDT
We'll need this test when mrbkap works on further document.domain related changes for beta8, so marking a blocker.
Comment 4 Johnny Stenback (:jst, 2010-11-12 16:23:02 PST
We don't need to worry about this for beta8, pushing to beta9.
Comment 5 christian 2011-01-04 15:43:10 PST
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
Comment 6 Mounir Lamouri (:mounir) 2011-01-04 15:52:31 PST
Re-setting the CC list assuming this change wasn't intentional.
Comment 7 christian 2011-01-04 18:14:18 PST
Fixing fields my automated script accidentally blanked. Apologies for the bugspam
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-21 10:29:36 PST
I'm keeping this as a blocker because I believe in test driven development yatta yatta, but tests don't need permission for approval so I question whether or not this even needs soft blocking status.

It needs to be done, absolutely, but does it need to keep us from shipping? I assert not!
Comment 9 Mounir Lamouri (:mounir) 2011-01-27 08:36:46 PST
Created attachment 507485 [details] [diff] [review]

Updated tests to make sure the test actually finish on trunk (currently 8 passes and 10 fails).
Comment 10 Mounir Lamouri (:mounir) 2011-01-27 08:48:01 PST
(In reply to comment #8)
> I'm keeping this as a blocker because I believe in test driven development
> yatta yatta, but tests don't need permission for approval so I question whether
> or not this even needs soft blocking status.

FWIW, the real purpose of the bug is to make trunk pass this test.
Comment 11 :Ehsan Akhgari (out sick) 2011-02-09 21:50:30 PST
We're past feature freeze.  This sounds like a non-blocker to me.
Comment 12 Johnny Stenback (:jst, 2011-02-11 10:20:07 PST
Yeah, we should get these tests in in a form where they pass, but we did change our minds on what the exact security policy is wrt document.domain. Either way, this is not a blocker any more due to the policy change.
Comment 13 Bobby Holley (busy) 2012-06-28 05:11:57 PDT
Created attachment 637462 [details] [diff] [review]
Better tests. v1

I spent some time with the tests on the bug this morning and found them inscrutable and impossible to extend. Here are some much cleaner ones.
Comment 14 Bobby Holley (busy) 2012-06-28 05:12:25 PDT
Note that we're now explicitly testing for the behavior I proposed here:
Comment 15 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-10 16:30:16 PDT
Comment on attachment 637462 [details] [diff] [review]
Better tests. v1

Review of attachment 637462 [details] [diff] [review]:

::: js/xpconnect/tests/chrome/test_documentdomain.xul
@@ +32,5 @@
> +  function startTest() {
> +
> +    // Grab all the content windows. We waive Xray here, and add it back with
> +    // XPCNativeWrapper whenever we pass references into content.
> +    var win1A = document.getElementById('test1A').contentWindow.wrappedJSObject;

All of this futzing around with .wrappedJSObject and XPCNativeWrapper shouldn't matter for the case where we're passing object around... I think you should be able to nuke it.
Comment 16 Bobby Holley (busy) 2012-07-12 01:12:17 PDT
Pushed to m-i:
Comment 17 Ed Morley [:emorley] 2012-07-12 09:35:56 PDT

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