Closed Bug 1322772 Opened 8 years ago Closed 8 years ago

Hang on navigating to image documents

Categories

(Remote Protocol :: Marionette, defect)

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

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1322772 - Poll for ready state on web progress stop state for image documents;
58 bytes, text/x-review-board-request
whimboo
: review+
impossibus
: review+
Details
Bug 1322772 - Avoid using infinite page load timeout;
58 bytes, text/x-review-board-request
whimboo
: review+
impossibus
: review+
Details
Bug 1322772 - Add extra safety checks for TLS cert tests;
58 bytes, text/x-review-board-request
whimboo
: review+
impossibus
: review+
Details
When using Marionette to navigate to image documents (.jpg/.gif/.png et al.), the images are loaded but a response is never sent to the client. It appears we are waiting for a DOMContentLoaded event to occur, but this never fires for image documents. We will need to use the web progress listener to short-circuit the navigation algorithm to return a little bit earlier for images than we would for regular HTML documents.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Blocks: webdriver
Attachment #8817741 - Flags: review?(dburns)
Attachment #8817742 - Flags: review?(dburns)
Attachment #8817743 - Flags: review?(dburns)
As whimboo is pointing out in https://bugzilla.mozilla.org/show_bug.cgi?id=1322227, the same behaviour can be seen for other documents that don’t produce DOMContentLoaded events. Examples include data:text/plain;base64,42 and other text/plain documents. The patches attached fixes image documents, but we should probably reverse the checks so that we _only_ look for DOMContentLoaded events when the document we start navigating to is an HTMLDocument. We can use the web progress listener to determine this.
Documents with text/plain are not affected by this because they are parsed as HTML (HTMLDocument). We need to special case ImageDocuments because they do not fire DOMContentLoaded events.
Comment on attachment 8817741 [details] Bug 1322772 - Poll for ready state on web progress stop state for image documents; https://reviewboard.mozilla.org/r/97950/#review100766 ::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:169 (Diff revision 3) > > def test_error_on_tls_navigation(self): > self.assertRaises(errors.InsecureCertificateException, > self.marionette.navigate, self.fixtures.where_is("/test.html", on="https")) > > + def test_image_document_to_html_document(self): Test name doesn't match what you are testing. Maybe we best even do HTML->Image->HTML. I also see duplication with the next test. I think we can combine both. ::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:171 (Diff revision 3) > self.assertRaises(errors.InsecureCertificateException, > self.marionette.navigate, self.fixtures.where_is("/test.html", on="https")) > > + def test_image_document_to_html_document(self): > + self.marionette.navigate(self.fixtures.where_is("test.html")) > + self.marionette.navigate(self.fixtures.where_is("1.gif")) Gifs... what old-style! :) Why not PNG for smaller sizes? ::: testing/marionette/listener.js:1045 (Diff revision 3) > > - // If the page we're at fired DOMContentLoaded and appears > - // to be the one we asked to load, then we definitely > - // saw the load occur. We need this because for error > - // pages, like about:neterror for unsupported protocols, > - // we don't end up opening a channel that our > + // If the page we're at fired DOMContentLoaded and appears to > + // be the one we asked to load, then we definitely saw the load > + // occur. We need this because for error pages, like about:neterror > + // for unsupported protocols, we don't end up opening a channel that > + // sour WebProgressListener can monitor. s/sour/our
Attachment #8817741 - Flags: review?(hskupin) → review+
Comment on attachment 8817742 [details] Bug 1322772 - Avoid using infinite page load timeout; https://reviewboard.mozilla.org/r/97952/#review100768 ::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:136 (Diff revision 3) > self.assertEqual("complete", state) > self.assertTrue(self.marionette.find_element(By.ID, "mozLink")) > > def test_error_when_exceeding_page_load_timeout(self): > with self.assertRaises(errors.TimeoutException): > - self.marionette.timeout.page_load = 0 > + self.marionette.timeout.page_load = 0.1 Ah, I assume that this is the reason why the test is failing that often for me locally. Great catch.
Attachment #8817742 - Flags: review?(hskupin) → review+
Comment on attachment 8817743 [details] Bug 1322772 - Add extra safety checks for TLS cert tests; https://reviewboard.mozilla.org/r/97954/#review100770 Not sure how this is related to this bug but it makes sense to check.
Attachment #8817743 - Flags: review?(hskupin) → review+
Comment on attachment 8817741 [details] Bug 1322772 - Poll for ready state on web progress stop state for image documents; https://reviewboard.mozilla.org/r/97950/#review100762 lgtm. All I found was typos. ::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:169 (Diff revision 3) > > def test_error_on_tls_navigation(self): > self.assertRaises(errors.InsecureCertificateException, > self.marionette.navigate, self.fixtures.where_is("/test.html", on="https")) > > + def test_image_document_to_html_document(self): Isn't this actually "html_document_to_image_document" because you start with test.html and and with 1.gif? ::: testing/marionette/listener.js:990 (Diff revision 3) > > onStateChange(webProgress, request, state, status) { > if (!(request instanceof Ci.nsIChannel)) { > return; > } > Re commit message: typo "reaces" ::: testing/marionette/listener.js:1045 (Diff revision 3) > > - // If the page we're at fired DOMContentLoaded and appears > - // to be the one we asked to load, then we definitely > - // saw the load occur. We need this because for error > - // pages, like about:neterror for unsupported protocols, > - // we don't end up opening a channel that our > + // If the page we're at fired DOMContentLoaded and appears to > + // be the one we asked to load, then we definitely saw the load > + // occur. We need this because for error pages, like about:neterror > + // for unsupported protocols, we don't end up opening a channel that > + // sour WebProgressListener can monitor. "sour"
Attachment #8817741 - Flags: review?(mjzffr) → review+
Comment on attachment 8817743 [details] Bug 1322772 - Add extra safety checks for TLS cert tests; https://reviewboard.mozilla.org/r/97954/#review100774
Attachment #8817743 - Flags: review?(mjzffr) → review+
Comment on attachment 8817742 [details] Bug 1322772 - Avoid using infinite page load timeout; https://reviewboard.mozilla.org/r/97952/#review100778 Odd. I wonder why the timeout was set to 0 in the first place.
Attachment #8817742 - Flags: review?(mjzffr) → review+
Comment on attachment 8817741 [details] Bug 1322772 - Poll for ready state on web progress stop state for image documents; https://reviewboard.mozilla.org/r/97950/#review100762 > Isn't this actually "html_document_to_image_document" because you start with test.html and and with 1.gif? You are right.
Comment on attachment 8817741 [details] Bug 1322772 - Poll for ready state on web progress stop state for image documents; https://reviewboard.mozilla.org/r/97950/#review100766 > Test name doesn't match what you are testing. Maybe we best even do HTML->Image->HTML. > > I also see duplication with the next test. I think we can combine both. Well spotted. Renamed it to `test_html_document_to_image_document`. I don’t think we can combine them because they are testing two disparate scenarios: the first that you can navigate from HTMLDocument → ImageDocument, and the second ImageDocument → ImageDocument. The second happens to reset the state to an HTMLDocument, but for clarity it would be better to see them both failing in case HTML → Image fails. In the latter case, Image → Image might only fail. > Gifs... what old-style! :) Why not PNG for smaller sizes? I salvaged the GIFs from https://github.com/mozilla/geckodriver/issues/350, but this is a valid point. We shouldn’t add unnecessary overhead to central. I experimented with making them data URIs, but that doesn’t let us have an easy way to look at the document title to determine if we reached the right destination, so I will replace them with tiny PNGs. > s/sour/our Fixed by someone else in the meantime, it appears after rebasing.
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6794051829d7 Poll for ready state on web progress stop state for image documents; r=maja_zf,whimboo https://hg.mozilla.org/integration/autoland/rev/b7d49ee7a72a Avoid using infinite page load timeout; r=maja_zf,whimboo https://hg.mozilla.org/integration/autoland/rev/8f2982511c22 Add extra safety checks for TLS cert tests; r=maja_zf,whimboo
https://hg.mozilla.org/mozilla-central/rev/6794051829d7 https://hg.mozilla.org/mozilla-central/rev/b7d49ee7a72a https://hg.mozilla.org/mozilla-central/rev/8f2982511c22
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
On behalf of Andreas I'm requesting the uplift of this test-only patch to aurora so it will be available with the next ESR.
status-firefox52: --- → affected
Whiteboard: [checkin-needed-aurora]
No longer blocks: 1331967
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: