Closed Bug 1322772 Opened 8 years ago Closed 7 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: 7 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: