Closed Bug 1314812 Opened 8 years ago Closed 8 years ago

Get rid of copy/pasted manual timeout in load functions

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(2 files)

Bug 1314812 - Remove code that manually times out loads in favor of relying on the test harness.
58 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1314812 - Remove manual timeout code in favor of letting the test harness do it for us.
58 bytes, text/x-review-board-request
Gijs
: review+
Details 
We have a ton of copy/pasted functions named |promiseTabLoadEvent| that all follow the same pattern. In particular, they including manual timeout code of the form:

setTimeout(() => { deferred.reject(/*...*/); }, 30000);

and then resolve a load promise based on a BrowserTestUtils.browserLoaded promise and |deferred.promise|. In bug 1301994, RyanVM and philor point out this code as bad, and they're right. During my investigation into that bug, I found that in cases where the load does time out for some reason, the test harness times out the test first, starts the next test and then at some point in a following test, our setTimeout fires and causes whatever test that happens to be running to fail.

So, we should get rid of these manual timeouts and let the test harness do its thing. Patches upcoming.
Comment on attachment 8806937 [details]
Bug 1314812 - Remove manual timeout code in favor of letting the test harness do it for us.

https://reviewboard.mozilla.org/r/90190/#review89868
Attachment #8806937 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8806936 [details]
Bug 1314812 - Remove code that manually times out loads in favor of relying on the test harness.

https://reviewboard.mozilla.org/r/90188/#review89870

::: browser/base/content/test/general/head.js:631
(Diff revision 1)
>    // Create two promises: one resolved from the content process when the page
>    // loads and one that is rejected if we take too long to load the url.

This comment

::: browser/base/content/test/general/head.js:638
(Diff revision 1)
>    // Promise.all rejects if either promise rejects (i.e. if we time out) and
>    // if our loaded promise resolves before the timeout, then we resolve the
>    // timeout promise as well, causing the all promise to resolve.

And this comment need to go. Ditto for all the other files here.

::: dom/plugins/test/mochitest/browser_bug1163570.js:12
(Diff revision 1)
>        if (event.originalTarget != tab.linkedBrowser.contentDocument ||
>            event.target.location.href == "about:blank" ||
>            (url && event.target.location.href != url)) {
>          return;
>        }
>        clearTimeout(timeout);

Missed a clearTimeout, and presumably we should nix `reject` ffrom the args here, too.
Attachment #8806936 - Flags: review?(gijskruitbosch+bugs) → review+
FWIW, I'm not sure to what degree this will meaningfully address any orange, but hey. Worth a shot.

(In reply to Blake Kaplan (:mrbkap) from comment #0)
> I found that in cases where the load does time out for some reason, the test
> harness times out the test first, starts the next test and then at some
> point in a following test, our setTimeout fires and causes whatever test
> that happens to be running to fail.

The timeouts are all much smaller than the 45s timeout that the test framework is supposed to be using, so I would be surprised if this was always the case.

The other thing that this code currently does is provide reasons as to why tests time out. Test framework timeouts are largely meaningless, you have to go dig through the test (and sometimes, like in the common "let's yield for a bunch of things while we do various actions and then test things afterwards", you might not know even after digging through it) to find out why it timed out.

I guess one thing we could do to all these functions is make them log something with `info` or similar indicating that we're creating a promise waiting for a load in tab X (or with url Y or whatever).
(In reply to :Gijs Kruitbosch from comment #5)
> FWIW, I'm not sure to what degree this will meaningfully address any orange,
> but hey. Worth a shot.

It probably won't change anything other than helping philor's blood pressure a little :-)

> The timeouts are all much smaller than the 45s timeout that the test
> framework is supposed to be using, so I would be surprised if this was
> always the case.

It's possible too that the test fails for other reasons before the timeout. I couldn't remember exactly what had happened in the logs I was reading other than seeing the setTimeout firing in other tests. I also often run tests under a C++ debugger which is supposed to disable timeouts and things like this often end up biting me since this timeout wouldn't be disabled.

> I guess one thing we could do to all these functions is make them log
> something with `info` or similar indicating that we're creating a promise
> waiting for a load in tab X (or with url Y or whatever).

Most of them already do this. When I've had to debug things like this, I've always ended up having to match the last ok/info to the test to figure out what we timed out or failed doing anyway, but I might be a glutton for punishment.
In bug 1299428 I'm completely removing all the promiseXXXLoad functions from browser/base/content/test/urlbar. I don't mind unbitrotting my patch fwiw, just a heads-up.
All of these special load functions can be easily replaced with BrowserTestUtils methods, also by volunteer contributors in mentored bugs.
(In reply to Marco Bonardo [::mak] from comment #9)
> In bug 1299428 I'm completely removing all the promiseXXXLoad functions from
> browser/base/content/test/urlbar. I don't mind unbitrotting my patch fwiw,
> just a heads-up.

What are the odds that we'd create patches on the same day? Sorry for the extra unbitrotting work!
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e9e418ab293
Remove code that manually times out loads in favor of relying on the test harness. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ab3b6f1e3dce
Remove manual timeout code in favor of letting the test harness do it for us. r=Gijs
(In reply to Blake Kaplan (:mrbkap) from comment #10)
> What are the odds that we'd create patches on the same day? Sorry for the
> extra unbitrotting work!

very high, looks like :D
https://hg.mozilla.org/mozilla-central/rev/4e9e418ab293
https://hg.mozilla.org/mozilla-central/rev/ab3b6f1e3dce
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Not all the comments seem to have been removed, e.g. https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/head.js#93-94 :-(

I'll audit and land a fix when I pop some other things off my queue
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2baab775c7e
followup nit: remove obsolete comments, rs=trivial,me,firebot, DONTBUILD
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: