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)
Tracking
()
REOPENED
mozilla26
People
(Reporter: KWierso, Assigned: bobowen)
References
Details
(Keywords: intermittent-failure, leave-open)
Attachments
(10 files, 10 obsolete files)
3.11 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
10.60 KB,
patch
|
Details | Diff | Splinter Review | |
11.20 KB,
patch
|
Details | Diff | Splinter Review | |
11.60 KB,
patch
|
Details | Diff | Splinter Review | |
61.28 KB,
patch
|
Details | Diff | Splinter Review | |
26.37 KB,
patch
|
Details | Diff | Splinter Review | |
7.55 KB,
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
15.42 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
13.42 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
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
Comment 5•11 years ago
|
||
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 :)
Assignee | ||
Comment 6•11 years ago
|
||
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
Assignee | ||
Comment 7•11 years ago
|
||
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
Assignee | ||
Comment 8•11 years ago
|
||
New try run: https://tbpl.mozilla.org/?tree=Try&rev=84fec5bb6217
Assignee | ||
Comment 9•11 years ago
|
||
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 10•11 years ago
|
||
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+
Comment 11•11 years ago
|
||
Olli, does this seem alright to you for now ?
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•11 years ago
|
Flags: needinfo?(bugs)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 23•11 years ago
|
||
Thanks Olli,
Would someone be able to push this to inbound for me?
Cheers,
Bob
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 31•11 years ago
|
||
(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
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 33•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 34•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 36•11 years ago
|
||
Reopening since it's still happening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 44•11 years ago
|
||
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.
Assignee | ||
Comment 45•11 years ago
|
||
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 hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 47•11 years ago
|
||
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 hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 50•11 years ago
|
||
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+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 53•11 years ago
|
||
(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
Comment 54•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 56•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Comment 57•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e22f0e9ed1db
https://hg.mozilla.org/releases/mozilla-beta/rev/c21f42bb22e6
status-firefox24:
--- → fixed
status-firefox25:
--- → fixed
status-firefox26:
--- → fixed
Target Milestone: mozilla25 → mozilla26
Comment 58•11 years ago
|
||
Pushed a follow-up for beta due to c21f42bb22e6 not being a complete uplift.
https://hg.mozilla.org/releases/mozilla-beta/rev/5954b0946800
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 60•11 years ago
|
||
(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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 78•11 years ago
|
||
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 79•11 years ago
|
||
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+
Assignee | ||
Comment 80•11 years ago
|
||
(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
Comment 81•11 years ago
|
||
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 83•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 86•11 years ago
|
||
(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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 116•11 years ago
|
||
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. :-)
Assignee | ||
Comment 117•11 years ago
|
||
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)
Assignee | ||
Comment 118•11 years ago
|
||
Attachment #8383100 -
Flags: review?(bugs)
Assignee | ||
Comment 119•11 years ago
|
||
Attachment #8383105 -
Flags: review?(bugs)
Assignee | ||
Comment 120•11 years ago
|
||
Attachment #8383107 -
Flags: review?(bugs)
Assignee | ||
Comment 121•11 years ago
|
||
Attachment #8383109 -
Flags: review?(bugs)
Assignee | ||
Comment 122•11 years ago
|
||
Attachment #8383112 -
Flags: review?(bugs)
Assignee | ||
Comment 123•11 years ago
|
||
Attachment #8383117 -
Flags: review?(bugs)
Assignee | ||
Comment 124•11 years ago
|
||
Attachment #8383119 -
Flags: review?(bugs)
Assignee | ||
Comment 125•11 years ago
|
||
Last patch, so here's a try push:
https://tbpl.mozilla.org/?tree=Try&rev=b4b99a2b162b
Attachment #8383125 -
Flags: review?(bugs)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 127•11 years ago
|
||
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 128•11 years ago
|
||
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.
Assignee | ||
Comment 129•11 years ago
|
||
(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?
Assignee | ||
Comment 130•11 years ago
|
||
(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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 132•11 years ago
|
||
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.)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 134•11 years ago
|
||
(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 135•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8383105 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8383107 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8383109 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8383112 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8383117 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8383119 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8383125 -
Flags: review?(bugs)
Assignee | ||
Comment 136•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8383096 -
Attachment is obsolete: true
Assignee | ||
Comment 137•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #8383109 -
Attachment is obsolete: true
Comment 138•11 years ago
|
||
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+
Assignee | ||
Comment 139•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Attachment #8383100 -
Flags: review?(bugs)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 142•11 years ago
|
||
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+
Assignee | ||
Comment 143•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #8383100 -
Attachment is obsolete: true
Comment 144•11 years ago
|
||
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-
Assignee | ||
Comment 145•11 years ago
|
||
(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)
Comment 146•11 years ago
|
||
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)
Assignee | ||
Comment 147•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8392154 -
Attachment is obsolete: true
Comment 148•11 years ago
|
||
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+
Assignee | ||
Comment 149•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8383105 -
Attachment is obsolete: true
Comment 150•11 years ago
|
||
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-
Assignee | ||
Comment 151•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #8392988 -
Attachment is obsolete: true
Comment 152•11 years ago
|
||
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+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 154•11 years ago
|
||
Thanks smaug, hopefully this one will be fairly similar.
Attachment #8395338 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Attachment #8383107 -
Attachment is obsolete: true
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 162•11 years ago
|
||
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-
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 164•11 years ago
|
||
(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)
Comment 165•11 years ago
|
||
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)
Assignee | ||
Comment 166•11 years ago
|
||
(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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•10 years ago
|
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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 195•10 years ago
|
||
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)
Comment 196•10 years ago
|
||
(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)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 235•9 years ago
|
||
Comment 236•9 years ago
|
||
(In reply to Pulsebot from comment #235)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ece67496e931
I'm just requesting a longer timeout here - see https://bugzilla.mozilla.org/show_bug.cgi?id=919246#c228.
Keywords: leave-open
Reporter | ||
Comment 237•9 years ago
|
||
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 246•8 years ago
|
||
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Updated•2 years ago
|
Severity: normal → S3
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•