Closed Bug 977625 Opened 6 years ago Closed 6 years ago

test_formaction_attribute.html hangs when running on local b2g-emulator

Categories

(Core :: DOM: Core & HTML, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
In attempting to get bug 935219 landed we ran into a try failure (timeout) on test_formaction_attribute.html. I was unable to reproduce that timeout locally, but I ran into another timeout while trying to reproduce locally.

In my case, the reason the test hangs is because the focus event never gets dispatched, and so after the main body of the test completes, the event-driven continuation bits don't run. I tracked down why the focus event doesn't get dispatched after putting focus in the input field, and I traced it to the line of code at [1]. At this point, mActiveWindow is null, which makes isElementInActiveWindow false, which makes sendFocusEvent false a few lines down, and that seems to go down a code path where we never send the focus event. I'm not sure why mActiveWindow is null here, but it seems that putting focus on the window before running the main part of the test seems to fix it.

Also note that when I run all the tests in the folder as a group, this test passes, so I assume this test is relying on behaviour from some previous test, which is bad.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp?rev=49993e01b351#1154
Try push to verify this doesn't break stuff on tbpl:

https://tbpl.mozilla.org/?tree=Try&rev=8ef464ac92b3
Assignee: nobody → bugmail.mozilla
Comment on attachment 8383058 [details] [diff] [review]
Patch

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

::: content/html/content/test/forms/test_formaction_attribute.html
@@ +67,5 @@
>  
>  /** Test for Bug 566160 **/
>  
>  SimpleTest.waitForExplicitFinish();
> +SimpleTest.waitForFocus(runTests, window);

According to [1], the second argument to waitForFocus() defaults to 'window', so it can be omitted.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#617
Comment on attachment 8383058 [details] [diff] [review]
Patch

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

::: content/html/content/test/forms/test_formaction_attribute.html
@@ +74,1 @@
>  });

Good catch! :)

However, addLoadEvent() is not needed if you use waitForFocus() and calling .focus() here doesn't sound useful. waitForFocus() is about focusing the window IIRC.

I think you should do:
SimpleTest.waitForFocus(runTests);

And remove:
addLoadEvent(function() {
  setTimeout(runTests, 0);
})
Attachment #8383058 - Flags: review?(mounir) → review-
Attached patch PatchSplinter Review
Attachment #8383058 - Attachment is obsolete: true
Attachment #8383759 - Flags: review?(mounir)
Comment on attachment 8383759 [details] [diff] [review]
Patch

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

r=me, thanks! :)
Attachment #8383759 - Flags: review?(mounir) → review+
https://hg.mozilla.org/mozilla-central/rev/0f87e8f8446c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.