Timeout error not returned when elapsing page loading timeout

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: ato)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {regression})

Version 3
mozilla52
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

Attachments

(14 attachments)

781 bytes, text/plain
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
(Reporter)

Description

2 years ago
Created attachment 8791172 [details]
testcase

Just noticed this while investigating a hidden test failure which appeared with my changes on bug 1156427.

We seem to constantly fail to raise the TimeoutException in those cases when any about: page has been loaded before. See the attached testcase for example (part of test_navigation.py).

For now I will disable this specific test via bug 1156427. We should not forget to re-enable it.
(Assignee)

Comment 1

2 years ago
I will take this.  hskupin caused me to open a can of worms, and I’ve found numerous other problems surrounding this.
Assignee: nobody → ato
Blocks: 721859
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8791305 [details]
Bug 1302707 - Correct input types for timeout function;

https://reviewboard.mozilla.org/r/78748/#review77374

::: testing/marionette/harness/marionette/tests/unit/test_implicit_waits.py:15
(Diff revision 1)
>  class TestImplicitWaits(MarionetteTestCase):
>      def testShouldImplicitlyWaitForASingleElement(self):
>          test_html = self.marionette.absolute_url("test_dynamic.html")
>          self.marionette.navigate(test_html)
>          add = self.marionette.find_element(By.ID, "adder")
> -        self.marionette.set_search_timeout("30000")
> +        self.marionette.set_search_timeout(30000)

It might be good to leave a test with string, so we can check that the type conversion will still work.
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8791306 [details]
Bug 1302707 - Replace uses of Marionette.timeouts with set_page_load_timeout in tests;

https://reviewboard.mozilla.org/r/78750/#review77376

::: testing/marionette/harness/marionette/tests/unit/test_navigation.py:128
(Diff revision 1)
>          self.assertEqual("complete", state)
>          self.assertTrue(self.marionette.find_element(By.ID, "mozLink"))
>  
>      def test_should_throw_a_timeoutexception_when_loading_page(self):
>          try:
> -            self.marionette.timeouts("page load", 0)
> +            self.marionette.set_page_load_timeout(0)

Keep in mind that this patch needs an update regarding my landed changes on autoland. The test is currently marked as skipped.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8791299 [details]
Bug 1302707 - Remove GeckoDriver#setScriptTimeout;

https://reviewboard.mozilla.org/r/78736/#review77946
Attachment #8791299 - Flags: review?(dburns) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8791300 [details]
Bug 1302707 - Remove GeckoDriver#setSearchTimeout;

https://reviewboard.mozilla.org/r/78738/#review77948
Attachment #8791300 - Flags: review?(dburns) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8791301 [details]
Bug 1302707 - Make GeckoDriver#timeouts spec compatible;

https://reviewboard.mozilla.org/r/78740/#review77950
Attachment #8791301 - Flags: review?(dburns) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8791303 [details]
Bug 1302707 - Add Marionette.set_page_load_timeout and adjust to new driver API;

https://reviewboard.mozilla.org/r/78744/#review77952
Attachment #8791303 - Flags: review?(dburns) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8791302 [details]
Bug 1302707 - Remove Marionette.timeouts from client;

https://reviewboard.mozilla.org/r/78742/#review77954
Attachment #8791302 - Flags: review?(dburns) → review+

Comment 18

2 years ago
mozreview-review
Comment on attachment 8791304 [details]
Bug 1302707 - Reuse Marionette.reset_timeouts in harness;

https://reviewboard.mozilla.org/r/78746/#review77956
Attachment #8791304 - Flags: review?(dburns) → review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8791305 [details]
Bug 1302707 - Correct input types for timeout function;

https://reviewboard.mozilla.org/r/78748/#review78456
Attachment #8791305 - Flags: review?(dburns) → review+
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8791305 [details]
Bug 1302707 - Correct input types for timeout function;

https://reviewboard.mozilla.org/r/78748/#review77374

> It might be good to leave a test with string, so we can check that the type conversion will still work.

This is a good idea, but I think I will add this as a dedicated test case so we remember why we did it, if you agree.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

2 years ago
mozreview-review
Comment on attachment 8791306 [details]
Bug 1302707 - Replace uses of Marionette.timeouts with set_page_load_timeout in tests;

https://reviewboard.mozilla.org/r/78750/#review79820
Attachment #8791306 - Flags: review?(dburns) → review+

Comment 33

2 years ago
mozreview-review
Comment on attachment 8791307 [details]
Bug 1302707 - Correct Marionette tests to match API changes;

https://reviewboard.mozilla.org/r/78752/#review79822
Attachment #8791307 - Flags: review?(dburns) → review+

Comment 34

2 years ago
mozreview-review
Comment on attachment 8794919 [details]
Bug 1302707 - Add test for Marionette:timeouts compat behaviour;

https://reviewboard.mozilla.org/r/81142/#review79824
Attachment #8794919 - Flags: review?(dburns) → review+

Comment 35

2 years ago
mozreview-review
Comment on attachment 8794923 [details]
Bug 1302707 - Unignore test that expects timeout error;

https://reviewboard.mozilla.org/r/81144/#review79826
Attachment #8794923 - Flags: review?(dburns) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Summary: No TimeoutException raised in navigate() with timeout set after an about page was loaded → Timeout error not returned when elapsing page loading timeout

Comment 48

2 years ago
mozreview-review
Comment on attachment 8795272 [details]
Bug 1302707 - Fix type check to allow page loading timeout of 0;

https://reviewboard.mozilla.org/r/81390/#review80248
Attachment #8795272 - Flags: review?(dburns) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 62

2 years ago
mozreview-review
Comment on attachment 8795772 [details]
Bug 1302707 - Set the default page loading- and script timeout;

https://reviewboard.mozilla.org/r/81712/#review80956
Attachment #8795772 - Flags: review?(dburns) → review+

Comment 63

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcd7283f53d6
Remove GeckoDriver#setScriptTimeout; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/b137cbc3015d
Remove GeckoDriver#setSearchTimeout; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/9f5ec882f0b9
Make GeckoDriver#timeouts spec compatible; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/a2e5784054a5
Remove Marionette.timeouts from client; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/2065cb35df80
Add Marionette.set_page_load_timeout and adjust to new driver API; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8495bc553c45
Reuse Marionette.reset_timeouts in harness; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/05f9545d263f
Correct input types for timeout function; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/646af81d6c2e
Replace uses of Marionette.timeouts with set_page_load_timeout in tests; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f665a904fc6b
Correct Marionette tests to match API changes; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/03a8529a7baa
Add test for Marionette:timeouts compat behaviour; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/6d9aac56c0fa
Unignore test that expects timeout error; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8ef405957cfe
Fix type check to allow page loading timeout of 0; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f8dbc2865d67
Set the default page loading- and script timeout; r=automatedtester
(Assignee)

Comment 65

2 years ago
Sheriffs: I’m not convinced this will apply cleanly to Aurora and Beta, but if it does please uplift as test-only changes.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
(Reporter)

Updated

2 years ago
Depends on: 1306848
Depends on: 1308453
As it had an serious impact on the beta release, next time, please don't bypass the process by "cheating" with a test-only approval.
(Reporter)

Comment 69

2 years ago
Just to add here... The update tests started to fail on Oct 1st, so a day after the push to mozilla-central. That's the time when the new code hit the Nightly or test package for the first time. 

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=fxup%20os%20x&filter-tier=1&filter-tier=2&filter-tier=3&bugfiler&fromchange=80a9c7007243ac4e931a8c4352723cbf840aff03

We should consider waiting for uplifts of Marionette changes until the next Nightly build has been successfully tested.
(Assignee)

Comment 70

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #68)
> As it had an serious impact on the beta release, next time, please don't
> bypass the process by "cheating" with a test-only approval.

Please hold your horses.  Detecting regressions caused by moving targets in out-of-tree code is impossible in the try integration process.  The changes made in these patches have no effect on end-users since they are hidden behind a flag to enable Marionette, which normally qualifies for the test-only approval.  The regression we are seeing here is in release automation, and not a regression that would affect users.

Anyway, the tl;dr here is that we are not so attached to the changes that we can’t back them out of beta.  Can I plead to someone’s good will to do that for me as I don’t have the experience doing that on a beta tree?
After discussing on IRC, we decided to backout the change:
https://hg.mozilla.org/releases/mozilla-beta/rev/5a5fc43f88e40f423cd052b2b537761a6a492da8

Then, please apologize if my tone "looked bad", I would like to restate that release management should be involved in decision when it might impact the release (automation or not). This is why I asked that the uplift approval process is used.
status-firefox50: fixed → disabled
(Assignee)

Comment 72

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #71)
> I would like to restate that
> release management should be involved in decision when it might impact the
> release (automation or not).

That is very reasonable and I apologise that I didn’t exert more diligence when writing this patch.

The core of the issue is that we have an inflexible situation with regards to making changes to the Marionette client, since it is being used for upgrade tests out-of-tree.  This means it is easy to detect regressions against the current Firefox built on try, but much harder to detect whether it continues to work reliably with older versions.

As :whimboo said on IRC, we have to use the newer Marionette client version because the upgrade tests might use new APIs that were not available in the known-compatible client with the Firefox that is being upgraded from.

Thanks for backing the patches out.
status-firefox50: disabled → wontfix
You need to log in before you can comment on or make changes to this bug.