Closed Bug 1099331 Opened 10 years ago Closed 10 years ago

Marionette's navigate errors on loading an about:blocked or about:error page

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

(Keywords: pi-marionette-server)

Attachments

(1 file, 1 obsolete file)

MozReview Request: 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
(deleted), text/x-review-board-request
automatedtester
: review+
Details
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.
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.
Attached file MozReview Request: bz://1099331/chmanchester (obsolete) (deleted) —
Attachment #8526217 - Flags: review?(dburns)
/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
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.
Mn was orange so have retriggered the tests a few times
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
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.
Comment on attachment 8526217 [details] MozReview Request: bz://1099331/chmanchester Not really expecting review until the wpt-reftest issue is resolved.
Attachment #8526217 - Flags: review?(dburns)
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.
:jgraham, do you have a preference how to deal with the issue I described in comment 11?
Flags: needinfo?(james)
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?
Flags: needinfo?(james)
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.
(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
(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.
(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.
(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).
(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?
(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.
Attachment #8526217 - Flags: review?(dburns)
/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
Attachment #8526217 - Flags: review?(dburns) → review+
https://hg.mozilla.org/mozilla-central/rev/0f7050eddd7d
Assignee: nobody → cmanchester
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
Flags: needinfo?(dburns)
(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
Flags: needinfo?(dburns)
actually I commented in Comment 15 what I wanted
Attachment #8526217 - Attachment is obsolete: true
Attachment #8618636 - Flags: review+

(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?

Flags: needinfo?(ato)

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.

Flags: needinfo?(ato)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: