Marionette's navigate errors on loading an about:blocked or about:error page
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(Not tracked)
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
(Keywords: pi-marionette-server)
Attachments
(1 file, 1 obsolete file)
I found this converting http://mxr.mozilla.org/mozmill-tests/source/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js for m21s. Marionette explicitly checks for about:blocked and about:error pages and returns an error when loading results in one. We could detect the resulting exception and check for it in the new test, but maybe this behavior should change now that we're explicitly testing these pages.
Comment 1•10 years ago
|
||
That's exactly what I'm working on right now locally. I have a test module which tests navigate for all kind of different pages. I hope to have it ready later today, maybe even before the meeting. So please don't spend your time on those specific navigate example tests.
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
/r/835 - Bug 1099331 - Allow navigation resulting in about:blocked or about:error pages without returning an error from the marionette server. Pull down this commit: hg pull review -r 52eb7b26ba2ccb191a996b3ee7cc028e6f2789b6
Comment 4•10 years ago
|
||
https://reviewboard.mozilla.org/r/833/#review387 Please can you push to try (already asked for this on IRC but just documenting) ::: testing/marionette/marionette-listener.js (Diff revision 1) > - sendOk(command_id); I am happy for this now but it deviates from how WebDriver currently operates but the spec doesnt describe all the behaviour properly. We *may* need to revisit this at a later date.
Assignee | ||
Comment 5•10 years ago
|
||
https://reviewboard.mozilla.org/r/833/#review395 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=81ae99fc0d73
Comment 6•10 years ago
|
||
Mn was orange so have retriggered the tests a few times
Assignee | ||
Comment 7•10 years ago
|
||
I think I see that failure on the emulators on m-c, but looks like I missed Mn on the other platforms in that try push, so here's another with more coverage: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f404ff830fb
Assignee | ||
Comment 8•10 years ago
|
||
Unexpected passes in that try run make me think web platform reftests may rely on the old behavior, will have to look into this some more.
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8526217 [details]
MozReview Request: bz://1099331/chmanchester
Not really expecting review until the wpt-reftest issue is resolved.
Updated•10 years ago
|
Assignee | ||
Comment 11•10 years ago
|
||
The test in question is html/semantics/text-level-semantics/the-wbr-element/wbr-element.html As far as I can tell, this test does a comparison against a page that doesn't exist. With the current behavior, the test has status error, but but with this patch the test passes, because the test is a '!=' comparison, and the error page isn't equivalent to the ref page. It does seem desirable to indicate something's gone wrong for a test like this, so I'm not sure just how to proceed with this.
Assignee | ||
Comment 12•9 years ago
|
||
:jgraham, do you have a preference how to deal with the issue I described in comment 11?
Comment 13•9 years ago
|
||
Well I guess the controversial approach is to say that the get command should return the status code and then the web-platform-tests harness could check that it got a 200 response for all the resources. I think in the short term I'm happy if you land this as is (with a metadata fixup) and we fix the test to not try and reference a missing file. Does that sound reasonable to you?
Comment 14•9 years ago
|
||
After the talk we had today I would be fine if an exception is thrown when a non-existent page is getting loaded and the HTTP response code is 404. That would actually indicate a problem in the navigate() call. If you cannot open a test page it doesn't make much sense to work with it. so best would be a specific Marionette exception like: try: self.marionette.navigate('http://example.org/nonexistent') except MarionettePageLoadException, e: do fallback code We should not do this if about:error has been explicitly requested, because loading it would be valid. Not sure what others think about it.
Comment 15•9 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #14) > After the talk we had today I would be fine if an exception is thrown when a > non-existent page is getting loaded and the HTTP response code is 404. That > would actually indicate a problem in the navigate() call. If you cannot open > a test page it doesn't make much sense to work with it. so best would be a > specific Marionette exception like: > > try: > self.marionette.navigate('http://example.org/nonexistent') > except MarionettePageLoadException, e: > do fallback code > > We should not do this if about:error has been explicitly requested, because > loading it would be valid. > > Not sure what others think about it. I am ok with this. marionette.navigate('about:error') # no exception marionette.navigate('http://www.domain.sometldthatdoesntexist') # Exception
Comment 16•9 years ago
|
||
(In reply to David Burns :automatedtester from comment #15) > I am ok with this. > > marionette.navigate('about:error') # no exception > marionette.navigate('http://www.domain.sometldthatdoesntexist') # Exception That doesn't seem like the proposal. I agree that if there is a DNS error it's OK to signal an error (and e.g. hrow in the client). I disagree that it's OK to do the same for a 404; using marionette to test the behaviour of a 404 response page is an entirely legitimate use case. For about:error specifically it sort of seems like loading that in Chrome context should work, but in Content context should fail. That reflects the underlying security division.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to James Graham [:jgraham] from comment #16) > (In reply to David Burns :automatedtester from comment #15) > > > I am ok with this. > > > > marionette.navigate('about:error') # no exception > > marionette.navigate('http://www.domain.sometldthatdoesntexist') # Exception > > That doesn't seem like the proposal. > > I agree that if there is a DNS error it's OK to signal an error (and e.g. > hrow in the client). I disagree that it's OK to do the same for a 404; using > marionette to test the behaviour of a 404 response page is an entirely > legitimate use case. The case that got me here was navigating to http://www.itisatrap.org/firefox/its-a-trap.html and ending up on about:blocked. What do we think of that case? My reaction was this patch, because it seemed preferable to let the browser do what it would do when a user navigates to this page without needing to deal with exceptions in the test. > > For about:error specifically it sort of seems like loading that in Chrome > context should work, but in Content context should fail. That reflects the > underlying security division. Navigate in chrome context isn't supported at all at the moment.
Comment 18•9 years ago
|
||
(In reply to James Graham [:jgraham] from comment #16) > I agree that if there is a DNS error it's OK to signal an error (and e.g. > hrow in the client). I disagree that it's OK to do the same for a 404; using > marionette to test the behaviour of a 404 response page is an entirely > legitimate use case. I wonder how you would be able to check if a given webpage your test has to load, was successfully loaded for testing purposes. Especially when there is no indication (like an exception) if it was successful or not. Is there a way to get the response code? Also keep in mind that we currently send an error in case of the interactive readyState case. (In reply to Chris Manchester [:chmanchester] from comment #17) > The case that got me here was navigating to > http://www.itisatrap.org/firefox/its-a-trap.html and ending up on > about:blocked. What do we think of that case? My reaction was this patch, > because it seemed preferable to let the browser do what it would do when a > user navigates to this page without needing to deal with exceptions in the > test. IMHO your code is doing the right thing in terms of handling the interactive readyState, but maybe it has to differentiate between about:error and all other about:* pages (in terms of sending ok or error)? > > For about:error specifically it sort of seems like loading that in Chrome > > context should work, but in Content context should fail. That reflects the > > underlying security division. > > Navigate in chrome context isn't supported at all at the moment. And it should not be allowed, given that you should not be able to replace the location. With that you would be able to e.g. load the content from the page info window inside the browser window, or even make the window totally busted (see bug 1093707).
Comment 19•9 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #18) > (In reply to James Graham [:jgraham] from comment #16) > > I agree that if there is a DNS error it's OK to signal an error (and e.g. > > hrow in the client). I disagree that it's OK to do the same for a 404; using > > marionette to test the behaviour of a 404 response page is an entirely > > legitimate use case. > > I wonder how you would be able to check if a given webpage your test has to > load, was successfully loaded for testing purposes. Especially when there is > no indication (like an exception) if it was successful or not. Is there a > way to get the response code? Also keep in mind that we currently send an > error in case of the interactive readyState case. Presumably you are generally going to interact with the page content in a way that would be totally broken if you loaded the 404 page rather than the page you expected (or the wrong page entirely due to e.g. a typo in the url). I agree that marionette should return the HTTP status code when a load is complete. I understand this idea is contentious in the webdriver community, for reasons that I don't pretend to understand. But we have freedom here to fulfill our use cases. > > > For about:error specifically it sort of seems like loading that in Chrome > > > context should work, but in Content context should fail. That reflects the > > > underlying security division. > > > > Navigate in chrome context isn't supported at all at the moment. > > And it should not be allowed, given that you should not be able to replace > the location. With that you would be able to e.g. load the content from the > page info window inside the browser window, or even make the window totally > busted (see bug 1093707). Right, but this is a testing tool. It's unsurprising that it's possible to shoot yourself in the foot with it. If you can't do this, is it possible to do useful things like check the content of about:blocked in a test?
Comment 20•9 years ago
|
||
(In reply to James Graham [:jgraham] from comment #19) > Presumably you are generally going to interact with the page content in a > way that would be totally broken if you loaded the 404 page rather than the > page you expected (or the wrong page entirely due to e.g. a typo in the url). Sure. The thing is that the failure you get in this case doesn't belong to the line of code in the test. As long as you don't look at screenshots, it will be unclear what the failure really is. > > And it should not be allowed, given that you should not be able to replace > > the location. With that you would be able to e.g. load the content from the > > page info window inside the browser window, or even make the window totally > > busted (see bug 1093707). > > Right, but this is a testing tool. It's unsurprising that it's possible to > shoot yourself in the foot with it. If you can't do this, is it possible to > do useful things like check the content of about:blocked in a test? We really do not talk about chrome scope here. This might be the confusion. about:blocked is a chrome page loaded in content scope. And its content is what we have to check with our tests.
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa5d774bcbf6
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 22•9 years ago
|
||
/r/835 - Bug 1099331 - Allow navigation resulting in about:blocked or about:error pages when explicitly requested without returning an error from the marionette server.;r=automatedtester Pull down this commit: hg pull review -r 6a4fa8a536f571ce34ab4e1a4f55eb5e8ea5f503
Updated•9 years ago
|
Comment 23•9 years ago
|
||
https://reviewboard.mozilla.org/r/833/#review849 Ship It!
Assignee | ||
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7050eddd7d
https://hg.mozilla.org/mozilla-central/rev/0f7050eddd7d
Comment 26•9 years ago
|
||
David, I think that parts of my comment 14 got lost. Maybe you can give me your feedback about this proposal in having a special Exception class for page load failures. > try: > self.marionette.navigate('http://example.org/nonexistent') > except MarionettePageLoadException, e: > do fallback code
Comment 27•9 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #26) > David, I think that parts of my comment 14 got lost. Maybe you can give me > your feedback about this proposal in having a special Exception class for > page load failures. > > > try: > > self.marionette.navigate('http://example.org/nonexistent') > > except MarionettePageLoadException, e: > > do fallback code sounds fine, raise a new bug to get it done
Comment 28•9 years ago
|
||
actually I commented in Comment 15 what I wanted
Assignee | ||
Comment 29•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
Comment 31•5 years ago
|
||
(In reply to James Graham [:jgraham] from comment #19)
(In reply to Henrik Skupin (:whimboo) from comment #18)
(In reply to James Graham [:jgraham] from comment #16)
I agree that if there is a DNS error it's OK to signal an error (and e.g.
hrow in the client). I disagree that it's OK to do the same for a 404; using
marionette to test the behaviour of a 404 response page is an entirely
legitimate use case.I wonder how you would be able to check if a given webpage your test has to
load, was successfully loaded for testing purposes. Especially when there is
no indication (like an exception) if it was successful or not. Is there a
way to get the response code? Also keep in mind that we currently send an
error in case of the interactive readyState case.Presumably you are generally going to interact with the page content in a
way that would be totally broken if you loaded the 404 page rather than the
page you expected (or the wrong page entirely due to e.g. a typo in the url).I agree that marionette should return the HTTP status code when a load is
complete. I understand this idea is contentious in the webdriver community,
for reasons that I don't pretend to understand. But we have freedom here to
fulfill our use cases.
The discussion happened about 4 years ago, but I just stumbled over an old todo item and found it again. I wonder if that would still be of interest now. If doable I would still like to see it. Andreas, what do you think about it?
Comment 32•5 years ago
|
||
To return the HTTP status code with the navigation response?
Technically a driver provide whatever additional fields it wants,
as long as they are prefixed with "moz:". I think it would make a
lot of users happy to have access to the HTTP status code.
Comment 33•5 years ago
|
||
I filed bug 1519110.
Updated•1 year ago
|
Description
•