850559 - test_imestate.html needs to call restoreNormalRefresh after advanceTimeAndRefresh

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/file/1b49fb552e18/dom/interfaces/base/nsIDOMWindowUtils.idl#l1130

The symptoms that I was seeing were that a subsequent mochitest could not open
a panel during a load event.  When it tried to open a panel during the load
event, the mochitest iframe continued to contain the previous mochitest document.  This was not just failure to paint the new document as expected from refresh driver issues, because text inputs in the previous test could be selected with the mouse and displayed a cursor.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: restoreNormalRefresh after advanceTimeAndRefresh

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I don't familiar with the symptom and the method which you are adding. Could you request review to somebody who is familiar with the method?

Comment 3

[Karl Tomlinson (:karlt)]

Masayuki, can you explain why this test calls advanceTimeAndRefresh(), please?
(I expect that won't be obvious to David, and I don't know)

Comment 4

[David Baron :dbaron:]

Comment on attachment 724307 [details] [diff] [review]
restoreNormalRefresh after advanceTimeAndRefresh

This is not the only test with this problem.  They seem to be:
content/events/test/test_bug574663.html
content/events/test/window_wheel_default_action.html
widget/tests/test_imestate.html

r=dbaron on fixing this.

I will disable any of these tests not fixed within 24 hours.

Comment 5

[David Baron :dbaron:]

These are regressions from:
https://hg.mozilla.org/mozilla-central/rev/3bc2919782a7
https://hg.mozilla.org/mozilla-central/rev/cf7a9685b8d8
https://hg.mozilla.org/mozilla-central/rev/927017027942

Comment 6

[David Baron :dbaron:]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #4)
> I will disable any of these tests not fixed within 24 hours.

To be clear, this is because these mess with later tests in a really bad way.

That said, I probably shouldn't have said that -- it's trivial to fix, and it should just be fixed, asap.

Comment 7

[David Baron :dbaron:]

And maybe we should rename advanceTimeAndRefresh to something scarier like controlRefreshTimeAdvanceAndRefresh?

Comment 8

[David Baron :dbaron:]

or controlRefreshTimeAndAdvanceAndRefresh?

Comment 9

[David Baron :dbaron:]

Attachment: Assert when a test leaves test control of refresh driver on.

Comment 10

[David Baron :dbaron:]

Attachment: Assert when a test leaves test control of refresh driver on.

Comment 11

[David Baron :dbaron:]

Attachment: Fix tests that call advanceTimeAndRefresh without calling restoreNormalRefresh.

Comment 12

[David Baron :dbaron:]

er, I wish bzexport didn't change assignee by default

Comment 13

[David Baron :dbaron:]

Try run with all three patches:
https://tbpl.mozilla.org/?tree=Try&rev=63572c3ff84e

Try run with only the assertion:
https://tbpl.mozilla.org/?tree=Try&rev=43bcec929438

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 724714 [details] [diff] [review]
Assert when a test leaves test control of refresh driver on.

Looks fine to me, but, if we do this, then we need to restoreNormalRefresh() in test_imestate.html before runEditableSubframeTests().

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> And maybe we should rename advanceTimeAndRefresh to something scarier like
> controlRefreshTimeAdvanceAndRefresh?

Fine, or perhaps something like advanceTimeRefreshAndPause?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (:karlt) from comment #3)
> Masayuki, can you explain why this test calls advanceTimeAndRefresh(),
> please?
> (I expect that won't be obvious to David, and I don't know)

I'm not sure. I probably stole the method from another test. I don't know the method what's doing. So, if you think it's not necessary, please remove it.

Comment 17

[Karl Tomlinson (:karlt)]

Mac and GTK tests pass (ignore the new test_panel_mouse_coords.xul I was testing) without advanceTimeAndRefresh().
https://tbpl.mozilla.org/?tree=Try&rev=4decd1d97fc1

Waiting for WINNT results:
https://tbpl.mozilla.org/?tree=Try&rev=623e7a0fc63e

Comment 18

[David Baron :dbaron:]

Comment on attachment 724714 [details] [diff] [review]
Assert when a test leaves test control of refresh driver on.

I think this approach isn't practical; I'm testing a patch to make mochitest report test failures if a test leaves things under refresh driver control (and also fix the situation).

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 724307 [details] [diff] [review]
restoreNormalRefresh after advanceTimeAndRefresh

Removed advanceTimeAndRefresh from test_imestate.html

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0eadfc2075

Comment 20

[David Baron :dbaron:]

Comment on attachment 724307 [details] [diff] [review]
restoreNormalRefresh after advanceTimeAndRefresh

Please don't obsolete the patch because it was checked in.

Comment 21

[David Baron :dbaron:]

Comment on attachment 724307 [details] [diff] [review]
restoreNormalRefresh after advanceTimeAndRefresh

Er, sorry, never mind.

Comment 22

[David Baron :dbaron:]

Attachment: Make mochitests check that the test didn't leave the refresh driver under test control.

Comment 23

[David Baron :dbaron:]

Try push with the mochitest patch and the other patches here:
https://tbpl.mozilla.org/?tree=Try&rev=abf260e55a65

Try push with just the mochitest patch without the patches in this bug:
https://tbpl.mozilla.org/?tree=Try&rev=b7e5a29ba76e

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 725444 [details] [diff] [review]
Make mochitests check that the test didn't leave the refresh driver under test control.

Review of attachment 725444 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but should we just add a more user-friendly API to the test harness that automatically cleans this up when the test is finished?

Comment 25

[David Baron :dbaron:]

This patch actually does clean things up, but it also makes the test fail -- I'd rather not have tests that use these APIs without understanding what they are (as happened here).

Comment 26

[David Baron :dbaron:]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #23)
> Try push with the mochitest patch and the other patches here:
> https://tbpl.mozilla.org/?tree=Try&rev=abf260e55a65
> 
> Try push with just the mochitest patch without the patches in this bug:
> https://tbpl.mozilla.org/?tree=Try&rev=b7e5a29ba76e

The first of these was green; the only failure in the second was:

12:07:48 INFO - 42069 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_imestate.html | test left refresh driver under test control

I'm not sure why the tests in attachment 724715 [details] [diff] [review] weren't causing failures there as well.

Comment 27

[David Baron :dbaron:]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #26)
> I'm not sure why the tests in attachment 724715 [details] [diff] [review]
> weren't causing failures there as well.

I guess it's because both are running in a separate *window*.  But that could change if we change what has a separate refresh driver and what shares a refresh driver.

Comment 28

[David Baron :dbaron:]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70969851084e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c63866c0b1d

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ac0eadfc2075
https://hg.mozilla.org/mozilla-central/rev/70969851084e
https://hg.mozilla.org/mozilla-central/rev/9c63866c0b1d

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I've not understood this bug's fix yet, however, I want to say, thank you for the fix. And I apologize that I couldn't touch this bug due to working on other bugs. I'll check this bug's change for new tests which I will write.

Thank you.