Hang on navigating to image documents

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug)

Version 3
mozilla53
Points:
---

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Blocks: webdriver
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1322227
(Assignee)

Updated

3 years ago
Attachment #8817741 - Flags: review?(dburns)
Attachment #8817742 - Flags: review?(dburns)
Attachment #8817743 - Flags: review?(dburns)
(Assignee)

Comment 8

3 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
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 13

2 years ago
mozreview-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/#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 14

2 years ago
mozreview-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 15

2 years ago
mozreview-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+
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
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

Comment 25

2 years ago
bugherder
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
Last Resolved: 2 years ago
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.
Whiteboard: [checkin-needed-aurora]
No longer blocks: 1331967
You need to log in before you can comment on or make changes to this bug.