Open Bug 885140 Opened 11 years ago Updated 2 years ago

Intermittent test_iframe_sandbox_navigation.html | Test timed out.

Categories

(Core :: DOM: Navigation, defect, P3)

Product:
Core
Component:
DOM: Navigation
Platform:
All
Linux
Type:
defect

Tracking

()

Status:
REOPENED
Milestone:
mozilla26
Tracking Flags:
Tracking Status
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: KWierso, Assigned: bobowen)

References

Details

(Keywords: intermittent-failure, leave-open)

Attachments

(10 files, 10 obsolete files)

Move calls to testAttempted() before mouse click.
3.11 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 6: iframe sandbox - child navigation tests
10.60 KB, patch
Details | Diff | Splinter Review
Part 7: iframe sandbox - our auxiliary navigation tests
11.20 KB, patch
Details | Diff | Splinter Review
Part 8: iframe sandbox - other auxiliary navigation tests
11.60 KB, patch
Details | Diff | Splinter Review
Part 9: Remove old iframe sandbox tests and helper files rendered redundant
61.28 KB, patch
Details | Diff | Splinter Review
Part 5: iframe sandbox - top navigation tests
26.37 KB, patch
Details | Diff | Splinter Review
Part 1: Notify when navigation blocked by sandboxing using the nsIObserverService v3
7.55 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
Part 2: iframe sandbox - self navigation tests v3
15.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 3: iframe sandbox - sibling navigation tests v3
13.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 4: iframe sandbox - parent navigation tests
23.24 KB, patch
smaug
: review-
Details | Diff | Splinter Review
https://tbpl.mozilla.org/php/getParsedLog.php?id=24342713&tree=Mozilla-Inbound 10:41:56 INFO - 137556 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_navigation.html | Test timed out. 10:41:56 INFO - args: ['/builds/slave/test/build/tests/bin/screentopng'] 10:41:56 INFO - Xlib: extension "RANDR" missing on display ":0". 10:42:07 INFO - SCREENSHOT: (See log) 10:42:07 INFO - 137557 INFO TEST-END | /tests/content/html/content/test/test_iframe_sandbox_navigation.html | finished in 312678ms
These tests were split as it was thought that intermittent timeouts were due to the size of the tests. I'll check the logs tonight and see if this is down to a particular test. When I was analysing this before it looked like it might be caused by a test that I changed from being one that only reported that it failed to one that reported when it passed as well. If there are indications that it could still be the same issue, I'll revert this test back to its original form. Cheers, Bob
Assignee: nobody → bobowencode
I've had a chance to look in my lunch hour at work and it is the same test that doesn't appear to have run (Test 6), so I think it must be causing the problem. It is one of two tests that call back up to the main test file for themselves to be modified and reloaded. The reload happens via a script invoked button click. So, I think that occasionally the second button click is overriding the first one so the document never gets reloaded. This obviously didn't cause a problem when the test only reported on failure, as if the test hadn't run properly you'd be none the wiser. By changing it to report on success could well explain this problem. I'll change it back to a report on failure only test ... hopefully tonight, if I get a chance. I intend to revisit these tests to make them serial. This was mainly to try and get them running on B2G, but it should also prevent other issues like this one and I could make this test report on success again. Cheers, Bob
Status: NEW → ASSIGNED
I made the changes. Unfortunately, I've had to do a full rebuild, which takes a while on my current set-up. I'll run the tests locally and then push to try as well, but it may have to wait until tomorrow. Cheers, Bob
Thanks very much for looking at this, Bob. I'd suggest that when you push to Try, re-run just the mochitest portion containing the problematic navigation test (mochitest-2 if I'm remembering correctly) a few times to see if the orange comes up.. I tried this before with the split tests and it looked OK, so this might not be foolproof, but might give a bit more confidence the problem is solved :)
Here's the try run: https://tbpl.mozilla.org/?tree=Try&rev=d6ae78964d4b Ian - I've used the settings you used to test this before. It was actually mochitest-1 for the Linux debug timeouts. It was mochitest-2 for the B2G problems ... not sure why there is a difference here. Cheers, Bob
Ah ... forgot to reduce the number of expected test passes. My compile failed, so I didn't have time to run the tests locally before work this morning. Again this will have to wait until I get back from work, but should be really quick to sort out. Cheers, Bob
Patch used for try run in Comment 8. No failures in the repeated runs, although of course with an intermittent bug this doesn't prove anything.
Attachment #766319 - Flags: review?(imelven)
Comment on attachment 766319 [details] [diff] [review] Revert Sandboxed Navigation Test 6 to only fail on error, to prevent time-outs. Review of attachment 766319 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, given the plans to revisit and improve the navigation tests in the future. As Bob describes it, it sounds more like a problem with the test (or a bug it's exposing), that the iframe sandbox functionality.
Attachment #766319 - Flags: review?(imelven) → review+
Olli, does this seem alright to you for now ?
Flags: needinfo?(bugs)
Ok to me. Sorry about delay.
Flags: needinfo?(bugs)
Thanks Olli, Would someone be able to push this to inbound for me? Cheers, Bob
(In reply to Bob Owen from comment #23) > Thanks Olli, > > Would someone be able to push this to inbound for me? Bob, if something is r+, you can put checkin-needed in the "keywords" section of the bug and the ever helpful sheriffs and checkin-needed elves will eventually check it in for you as well.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ff04bb944aa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Reopening since it's still happening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Two new try runs where the calls to indicate that the "fails if bad" tests have run have been moved. https://tbpl.mozilla.org/?tree=Try&rev=cc0ce99d4057 https://tbpl.mozilla.org/?tree=Try&rev=3fbbd93870b4 If this doesn't work then I'll remove these calls completely and not try to detect if the "fails if bad" tests have run.
Patch for try runs in Comment 44. I originally added these testAttempted() calls for the tests that only reported anything if they failed, because sometimes they weren't all being run before the other tests had passed and SimpleTest.finish() had been called. If I don't get a timeout in the try runs after several re-runs then I'll progress this patch. Otherwise, as I said in Comment 44, I'll take out this testAttempted stuff altogether.
Attachment #766319 - Attachment is obsolete: true
Comment on attachment 787133 [details] [diff] [review] Move calls to testAttempted() before mouse click, in case it isn't getting called. Only two oranges in the try runs and neither for this test. Olli would you mind reviewing for me?
Attachment #787133 - Flags: review?(bugs)
Comment on attachment 787133 [details] [diff] [review] Move calls to testAttempted() before mouse click, in case it isn't getting called. Hackish
Attachment #787133 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #50) > Comment on attachment 787133 [details] [diff] [review] > Move calls to testAttempted() before mouse click, in case it isn't getting > called. > > Hackish Thanks Olli. I'll take that as a compliment :) I know it's a bit of a stab in the dark, but it's really difficult to get to the root cause of this issue, because I've not been able to reproduce it outside of the build servers.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23135514a47b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 906814
Pushed a follow-up for beta due to c21f42bb22e6 not being a complete uplift. https://hg.mozilla.org/releases/mozilla-beta/rev/5954b0946800
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to TBPL Robot from comment #59) > edmorley > https://tbpl.mozilla.org/php/getParsedLog.php?id=28269128&tree=Mozilla- > Inbound > Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound opt test mochitest-1 on > 2013-09-23 22:33:33 > revision: d49ebcf184c0 > slave: talos-r4-snow-027 > > 145292 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_iframe_sandbox_navigation.html | Test > timed out. > 148358 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_object_plugin_nav.html | Test timed > out. > 148359 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_object_plugin_nav.html | > [SimpleTest.finish()] waitForFocus() was called a different number of times > from the number of callbacks run. Maybe the test terminated prematurely -- > be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0 > 150680 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_video_wakelock.html | Test timed out. > ReferenceError: makeURI is not defined > ReferenceError: makeURI is not defined > ReferenceError: makeURI is not defined > ReferenceError: makeURI is not defined Looking at the logs this appears to be a timeout in a different sub-test. It's not one that has changed recently. The screen-shot seems to indicate that file_iframe_sandbox_e_if4.html just failed to load, so I'm not sure how this can be addressed. I'll wait to see if this looks like a recurring problem, before I investigate any further.
I've run these tests about a hundred times on try for OS X 10.6/7 Opt, but while there have been other timeouts, none for these tests. https://tbpl.mozilla.org/?tree=Try&rev=6bdf51f5bb89 Anyway, I've found another instance, where I'm calling testAttempted() after the click, which has caused problems before, so I've moved it. I've also moved the iframe it is trying to navigate, before it in the main test just in case this is ever causing problems. New try run with these changes here: https://tbpl.mozilla.org/?tree=Try&rev=9e17cc8458bb
Attachment #787133 - Attachment is obsolete: true
Attachment #830256 - Flags: review?(bugs)
Comment on attachment 830256 [details] [diff] [review] Move calls to testAttempted() before mouse click. I guess we can try this.
Attachment #830256 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #79) > Comment on attachment 830256 [details] [diff] [review] > Move calls to testAttempted() before mouse click. > > I guess we can try this. Thanks Olli.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38bcddde0516
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
:(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Wes Kocher (:KWierso) from comment #85) > :( I second that. I've noticed that there are some other bugs 919246, 922627, 928678, 929322, 932188 that are mainly about sandbox tests timing out on OSX 10.6 and 10.7. All of these tests do a lot of page loading, either in iframes, frames etc. or newly opened windows / tabs. I wonder if they could be hitting the same problem as bug 921635, this mentions something about loading pages too fast.
I'm about to upload a re-write of the iframe sandbox navigation tests. This has taken me a lot longer than I anticipated and the patch was pretty big, so I've split it into a series of smaller patches today, that are hopefully easier to review. The basic approach was to rename and extend the tests for bug 785310, as they cover very similar behaviour. For each set of tests, I've commented the old tests, so you can see what they replace. The last patch will clean up the old tests. This covers both lots of navigation tests, the navigation2 ones have a separate bug 919246 for their intermittent failures. Hopefully this re-write, which runs the tests in series, will not have the same timeout problems and should also run on b2g. :-)
Blocks: 919246
Depends on: 174349
This is to allow the testing of asynchronous navigation (e.g. via anchor) to be more robust. It also means that other navigation testing can be more rigorous, as we can check to see that the navigation was blocked by sandboxing when an exception is thrown.
Attachment #8383096 - Flags: review?(bugs)
Attachment #8383100 - Flags: review?(bugs)
Attachment #8383105 - Flags: review?(bugs)
Attachment #8383107 - Flags: review?(bugs)
Attachment #8383109 - Flags: review?(bugs)
Comment on attachment 8383096 [details] [diff] [review] Part 1: Notify when navigation blocked by sandboxing using the nsIObserverService No need to add anything to nsSandboxFlags.h. Just pass the string to NotifyObservers And I think we should notify only in one place. Somehow need to rename IsSandBoxedFrom and make the method do the notifying. Could we rename the method to SandboxFrom() ?
Attachment #8383096 - Flags: review?(bugs) → review-
Comment on attachment 8383100 [details] [diff] [review] Part 2: iframe sandbox - self navigation tests >+ // passes if good *** Re-written *** I have no idea what this change means. >- // passes if good, fails if bad >+ // passes if good, fails if bad *** Re-written *** or this hmmm, so how am I suppose to compare whether useful stuff is removed.. This all is going to be hard to review.
(In reply to Olli Pettay [:smaug] from comment #127) > Comment on attachment 8383096 [details] [diff] [review] > Part 1: Notify when navigation blocked by sandboxing using the > nsIObserverService > > No need to add anything to nsSandboxFlags.h. > Just pass the string to NotifyObservers OK, I'll change. > And I think we should notify only in one place. > Somehow need to rename IsSandBoxedFrom and make the method > do the notifying. > Could we rename the method to SandboxFrom() ? I did think about this, but didn't in case this function is ever used for other reasons. Also given the lack of exceptions the actual check, as to whether to block or not, is always going to take place outside this function. The check in nsWindowWatcher::OpenWindowInternal is only there because the way it currently works means that an existing window's state can be changed before we attempt to navigate it in InternalLoad. This will eventually go once I've got round to fixing bug 960545. What do you think? If you still think it should move into the function then perhaps a name of EnforceSandbox() and have it return an nsresult?
(In reply to Olli Pettay [:smaug] from comment #128) > Comment on attachment 8383100 [details] [diff] [review] > Part 2: iframe sandbox - self navigation tests > > >+ // passes if good *** Re-written *** > I have no idea what this change means. > > >- // passes if good, fails if bad > >+ // passes if good, fails if bad *** Re-written *** > or this > > hmmm, so how am I suppose to compare whether useful stuff is removed.. > This all is going to be hard to review. Sorry, I should have probably posted Comment 116 after all the patches. This was actually an attempt to make it easier to review. For each patch I've put a comment on the tests that have been re-written within the patch. The last patch cleans up the old tests, removing all the ones that are no longer needed.
So, since it is not clear what will happen with bug 174349, these tests should not rely on that. (And I'll try to get to the reviewing soon.)
(In reply to Bob Owen (:bobowen) from comment #129) > I did think about this, but didn't in case this function is ever used for > other reasons. But it isn't. At least currently. > What do you think? > If you still think it should move into the function then perhaps a name of > EnforceSandbox() and have it return an nsresult? That sounds fine.
Comment on attachment 8383100 [details] [diff] [review] Part 2: iframe sandbox - self navigation tests Since these patches need small tweaking anyway to not rely on bug 174349, I should look at them after the changes.
Attachment #8383100 - Flags: review?(bugs)
Attachment #8383105 - Flags: review?(bugs)
Attachment #8383107 - Flags: review?(bugs)
Attachment #8383109 - Flags: review?(bugs)
Attachment #8383112 - Flags: review?(bugs)
Attachment #8383117 - Flags: review?(bugs)
Attachment #8383119 - Flags: review?(bugs)
Attachment #8383125 - Flags: review?(bugs)
Renamed IsSandboxedFrom to EnforceSandbox, which now notifies and returns the error itself. Also changed the error to a DOM SecurityError as Hixie has now changed the spec to explicity define which error should be thrown. I'll wait until you're happy with this before asking for reviews on the others.
Attachment #8387643 - Flags: review?(bugs)
Attachment #8383096 - Attachment is obsolete: true
In the tests where the child can successfully navigate top, I've changed it to assume that no exception is a pass. This is because the window.opener gets set to the child and so I can't send the message back to the test window. This removes the dependency on bug 174349.
Attachment #8383109 - Attachment is obsolete: true
No longer depends on: 174349
Comment on attachment 8387643 [details] [diff] [review] Part 1: Notify when navigation blocked by sandboxing using the nsIObserverService v2 >@@ -482,18 +482,19 @@ nsWindowWatcher::OpenWindowInternal(nsID > // The state of the window can change before this call and if we are blocked > // because of sandboxing, we wouldn't want that to happen. > nsCOMPtr<nsPIDOMWindow> parentWindow = do_QueryInterface(aParent); > nsCOMPtr<nsIDocShell> parentDocShell; > if (parentWindow) { > parentDocShell = parentWindow->GetDocShell(); > if (parentDocShell) { > nsCOMPtr<nsIDocShell> foundDocShell = do_QueryInterface(newDocShellItem); >- if (parentDocShell->IsSandboxedFrom(foundDocShell)) { >- return NS_ERROR_DOM_INVALID_ACCESS_ERR; >+ rv = parentDocShell->EnforceSandbox(foundDocShell); >+ if (NS_FAILED(rv)) { >+ return rv; 2 spaces for indentation.
Attachment #8387643 - Flags: review?(bugs) → review+
r=smaug - from comment 138 Thanks smaug Sorry about that, I spotted it just after I'd uploaded, but thought I wouldn't cause extra churn in case there were other changes. Copy and paste bug from nsDocShell. :(
Attachment #8387643 - Attachment is obsolete: true
Attachment #8387762 - Flags: review+
Attachment #8383100 - Flags: review?(bugs)
Comment on attachment 8383100 [details] [diff] [review] Part 2: iframe sandbox - self navigation tests This is still super hard to review, since one needs to figure out which test this is actually replacing and looking at that test in a different patch, and try to do some comparison. Could we not just split test_iframe_sandbox_navigation.html and test_iframe_sandbox_navigation2.html to several files, so that each tests function doTest()s about are run separately or something. But if this new setup is really needed... I guess the patch looks ok except I don't understand the use of .blockedBySandbox. Why do we need to complicate the setup to just get different kind of message for ok(false, foo) ? So remove that and perhaps let a testcase to be without any onBlockedBySandbox callback.
Attachment #8383100 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #142) > Comment on attachment 8383100 [details] [diff] [review] > Part 2: iframe sandbox - self navigation tests > > This is still super hard to review, since one needs to figure out which test > this is actually replacing and looking at that test in a different patch, > and try to do some comparison. Thanks for persevering with this smaug. In an attempt to make your life easier I've updated the patch to take out the tests properly as I go, instead of just adding a comment. This includes removing any helper files, so hopefully you can see the mechanism of the test. Although some of the common helper files will have to wait until they are not used anymore. I'm conscious that there are another seven of these patches, so if this is an improvement, I'm happy to do the same for the others. > Could we not just split test_iframe_sandbox_navigation.html and > test_iframe_sandbox_navigation2.html to several > files, so that each tests function doTest()s about are run separately or > something. I did consider this. However, it means you'd have an even larger amount of files to achieve the testing. I think it would make the tests incoherent and make it difficult to work out what is tested and what isn't. The new tests make that much easier and also make it easy to add new test cases if required. It would also probably make them run a fair bit more slowly. I know reviewing the transition is painful, but I definitely think the new tests are much better. > But if this new setup is really needed... I guess the patch looks ok except > I don't understand the use of .blockedBySandbox. Why do we need to > complicate the setup to just get different kind of message for ok(false, > foo) ? > So remove that and perhaps let a testcase to be without any > onBlockedBySandbox > callback. Ah, yes, this is a bit of a hangover from the other tests, which were copied to create the self navigation ones (which I'd missed for bug 785310). It just so happens that I split them in alphabetical order. In the other tests, it makes more sense because I can fail the test if the exception wasn't because of sandboxing. But here we're never expecting an exception, so I've done as you suggest and removed it for these tests.
Attachment #8392154 - Flags: review?(bugs)
Attachment #8383100 - Attachment is obsolete: true
Comment on attachment 8392154 [details] [diff] [review] Part 2: iframe sandbox - self navigation tests You're changing the tests here. file_iframe_sandbox_d_if1.html tests that navigation to file_iframe_sandbox_navigation_pass.html works. file_self_navigation.html tests navigation from file_self_navigation.html -> file_self_navigation.html# (This way the patches are easier to review.)
Attachment #8392154 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #144) > Comment on attachment 8392154 [details] [diff] [review] > Part 2: iframe sandbox - self navigation tests > > You're changing the tests here. > file_iframe_sandbox_d_if1.html tests that navigation to > file_iframe_sandbox_navigation_pass.html works. > > file_self_navigation.html tests navigation from > file_self_navigation.html -> file_self_navigation.html# Well, the detail of the tests are different, but what is being tested is the same. So, it navigates back to the same URL, but that doesn't really matter. We are testing who can navigate whom, as long as a navigation occurs the URI doesn't matter as far as the sandboxing is concerned. Doing it this way saves resetting the test document in the iframe (or window in other tests) each time. So, the tests are faster. Also, I'm not sure why you have a "#" on the end above. The only time there is a fragment identifier is when we explicitly set location.hash in Test 4. > (This way the patches are easier to review.) Good, I'll do this for the other patches, when I get this one through. :-)
Flags: needinfo?(bugs)
I should have used ' and not #, just to indicate a new load. Docshell handles same-url vs not-same-url navigations in many places in different way, so we should hopefully have tests for both.
Flags: needinfo?(bugs)
As per our irc, I've added the test number as a query string to give not-same-url tests, like the originals. I've also cut it down to just re-create the two tests I am removing, to make it less of a hassle to review. I'll do the same for the other patches, before review. Once these are in and I've hopefully squashed the oranges, I'll file a follow up bug to add in the extra tests in each case, including ones for same-url navigation. Thanks for your patience with this smaug.
Attachment #8392918 - Flags: review?(bugs)
Attachment #8392154 - Attachment is obsolete: true
Comment on attachment 8392918 [details] [diff] [review] Part 2: iframe sandbox - self navigation tests v3 I assume file_on_block_observer.js just overrides the same file from the preview test.
Attachment #8392918 - Flags: review?(bugs) → review+
There's a bit of file renaming in this one as well, but hopefully that won't be too distracting. I've also changed it to reference the original iframe sandbox bug, as this is the feature that these are acutally testing. I've not added the parts where I check the observer even when I get an exception, so I know that the navigation really has been blocked by sandboxing. Trying to keep the changes to a minimum. I do think it is worth doing, but I'll add all of that in the follow up bug.
Attachment #8392988 - Flags: review?(bugs)
Attachment #8383105 - Attachment is obsolete: true
Comment on attachment 8392988 [details] [diff] [review] Part 3: iframe sandbox - sibling navigation tests v2 Since tests 1-4 block, couldn't also those use onBlockedBySandbox: passOnBlock ? I don't understand the setup for iframe loading handling. For sibling we use message events and for testIframe load event. Why ?
Attachment #8392988 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #150) > Comment on attachment 8392988 [details] [diff] [review] > Part 3: iframe sandbox - sibling navigation tests v2 > > Since tests 1-4 block, couldn't also those use > onBlockedBySandbox: passOnBlock ? Well, almost. You could use passOnBlock and have an empty catch block for the eval. This would be fine if the test were all passing, but if something broke them in a way that still threw an exception, but not because of sandboxing, the tests would hang and then time out. This is where I'd added a different callback to just record the notification of the block on the testCase, so we could check in the exception handler that the navigation really had been blocked because of sandboxing. This is a real improvement, because with just a plain exception handler, if you had a syntax error (for example) in the script, an exception would still be thrown and the test would pass. I'd taken this all back out of the patch to try and ease the reviews. I was going to drop it back in again in the follow-up bug, but I've put it back in now, if you're happy with the extra changes. I've also added on the dummy query string, as I'm making bigger changes to the existing tests anyway. > I don't understand the setup for iframe loading handling. > For sibling we use message events and for testIframe load event. > Why ? I tried using window.onload and the load events for the iframes, but I was getting timing issues on try, where the postMessage from the initial load of the sibling iframe was interfering with the testing. You're right though this is not obvious, as demonstrated by the fact that I had to go back to the try runs to remind myself exactly why I'd done it this way. :-) So, I've added a comment to explain.
Attachment #8393382 - Flags: review?(bugs)
Attachment #8392988 - Attachment is obsolete: true
Comment on attachment 8393382 [details] [diff] [review] Part 3: iframe sandbox - sibling navigation tests v3 I guess. Odd stuff.
Attachment #8393382 - Flags: review?(bugs) → review+
Thanks smaug, hopefully this one will be fairly similar.
Attachment #8395338 - Flags: review?(bugs)
Attachment #8383107 - Attachment is obsolete: true
Comment on attachment 8395338 [details] [diff] [review] Part 4: iframe sandbox - parent navigation tests I don't understand this. We remove 3) but what replaces it? I don't see a case when there is sandbox="allow-scripts" test added.
Attachment #8395338 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #162) > Comment on attachment 8395338 [details] [diff] [review] > Part 4: iframe sandbox - parent navigation tests > > I don't understand this. We remove 3) but what replaces it? I don't see a > case when there is sandbox="allow-scripts" test added. It's replaced by test 5, which admittedly is a slightly stronger test, with all the keywords that affect navigation by script - allow-scripts, allow-same-origin and allow-top-navigation. I know that we changed to just replacing the old tests, but to exactly match the flags, I'd have to do something like add another child iframe into the parent with just allow-scripts (and another with allow-same-origin as well for some of the other tests) and then add extra information to the testCase to tell it which child iframe to use. This all seems like a lot of extra work when I'm replacing it with a similar, but stronger test. Also, I'd probably want to change it to this in a follow up bug anyway. I don't think it would be worth testing all the combinations of flags. Did that all make sense? :)
Flags: needinfo?(bugs)
Well, having "allow-scripts, allow-same-origin and allow-top-navigation" is quite different to just "allow-scripts". If I wouldn't know the implementation at all, I could imagine those going through quite different code paths. Because this stuff is complicated, might be good to not lose simple tests.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #165) > Well, having "allow-scripts, allow-same-origin and allow-top-navigation" is > quite different to > just "allow-scripts". If I wouldn't know the implementation at all, I could > imagine those > going through quite different code paths. > Because this stuff is complicated, might be good to not lose simple tests. I agree that using knowledge about the implementation is generally a bad thing. But in this case my reasoning is: Sandboxed navigation of your parent should always be blocked (assuming it is not top). So, testing it with all of the keywords that are at all likely to permit the navigation seems like the best test. If it is blocked with this, it is then fairly reasonable to assume that it will be blocked if more restrictions are in place. The alternative is to run the tests with all combinations of keywords. Even if you restrict this to the ones that you think might conceivably make a difference, given the number of tests there are already, I think this could quickly get out of hand.
Summary: Intermittent TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_navigation.html | Test timed out. → Intermittent test_iframe_sandbox_navigation.html | Test timed out.
I'm going to pick this up again. smaug, would you have a look at my reply in comment 166? The context was that the old test, that I am replacing, had different sandbox keywords applied. I think that the replacement is at least as good and fits with the existing tests I am extending. If we think that more keyword combinations should be tested, this should maybe be a follow up.
Flags: needinfo?(bugs)
(In reply to Bob Owen (:bobowen) from comment #166) > The alternative is to run the tests with all combinations of keywords. > Even if you restrict this to the ones that you think might conceivably make > a difference, given the number of tests there are already, I think this > could quickly get out of hand. Why you think that? There aren't that many keywords. If we automate the test enough (and not write test case for each combination manually), that should work I think.
Flags: needinfo?(bugs)
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: