Closed Bug 620789 Opened 11 years ago Closed 8 years ago

Intermittent browser_bug581253.js | Test timed out followed by Found a tab after previous test timed out

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0b12

People

(Reporter: philor, Assigned: hiro)

References

Details

(Keywords: intermittent-failure)

Attachments

(5 files, 12 obsolete files)

Use inline function (checked-in)
871 bytes, patch
mak
: review+
Details | Diff | Splinter Review
Check PlacesStarButton._ignoreClicks instead of starButton.hidden (checked-in)
787 bytes, patch
mak
: review+
Details | Diff | Splinter Review
Just clear timeout. (Checked-in)
1.75 KB, patch
mak
: review+
Details | Diff | Splinter Review
review in form of patch [Checked in: Comment 465]
927 bytes, patch
mak
: review+
dietrich
: approval2.0+
Details | Diff | Splinter Review
Proof of that scenario v2
1.71 KB, patch
Details | Diff | Splinter Review 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1292967961.1292969019.4459.gz#err5
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/12/21 13:46:01
s: talos-r3-fed64-029

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Waiting for star button change.
(x a million)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Test timed out
(screenshot)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Found a tab after previous test timed out: data:text/plain,nothing%20but%20plain%20text
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_410196_paste_into_tags.js | node uri's are the same - Got http://example.com/, expected http://mozilla.com/
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | tags field value was set
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | the bookmark for the test url has been removed

Not timing out would be good, too, but using registerCleanupFunction() to not mess with other tests running after you were supposed to stop would be a great first step.
Attached patch Part of fix for bug 630789 (obsolete) — — Splinter Review 
I totally agree with Phil's opinion. We should do first clean up browser_bug581253.js state before the next test run.
Attachment #504333 - Flags: review?
Attachment #504333 - Flags: review? → review?(mak77)
Comment on attachment 504333 [details] [diff] [review]
Part of fix for bug 630789

So, I'm fine with doing better cleanup while we figure out the cause, but this patch won't make anything useful as it is, since some of next tests are failing on bookmarks, and this patch is removing history.

You have to add a PlacesUtils.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);
to the cleanup function.
So finally this patch will only fix browser_410196_paste_into_tags.js when this one timeouts, nothing more.

Regarding the orange, looks like we never get the popupshown event, either the click is dismissed, something stops propagation or the popup does not open for any other reason (a focus change?).

Notice that in each case, I don't understand the relation between this failure and the globalwarnings or addons tests failures, ideally there is none.
Looks like there could be some issue with arrow panels? all of the failing tests seem to try to show a arrow panel and they are failing doing so.
Attachment #504333 - Flags: review?(mak77) → review-
Attached patch Revised patch (obsolete) — — Splinter Review 
Additional call of removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId).
Attachment #504333 - Attachment is obsolete: true
Attachment #504582 - Flags: review?(mak77)
(In reply to comment #86)

> So, I'm fine with doing better cleanup while we figure out the cause, but this
> patch won't make anything useful as it is, since some of next tests are failing
> on bookmarks, and this patch is removing history.
> 
> You have to add a
> PlacesUtils.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);
> to the cleanup function.

Thank you for your keen eyes! I added removeFolderChildren in the new patch.

> So finally this patch will only fix browser_410196_paste_into_tags.js when this
> one timeouts, nothing more.
>
> Regarding the orange, looks like we never get the popupshown event, either the
> click is dismissed, something stops propagation or the popup does not open for
> any other reason (a focus change?).

Yes. I am suspecting bug 624097 and bug 551540 are related to be this issue, but those bugs are only on Linux. But anyway, I could confirm those bugs on Linux, (Unfortunately I can not reproduce the timeout of this issue though.) and suspect there are some focus manager issues on Linux.
Comment on attachment 504582 [details] [diff] [review]
Revised patch

>diff --git a/browser/base/content/test/browser_bug581253.js b/browser/base/content/test/browser_bug581253.js

>+function waitForClearHistoryAndBookmarks()
> {
>+  // Though this function is not in test itself,
>+  // we have to wait for cleaning up history so
>+  // we need to use waitForExplicitFinish here.
>+  waitForExplicitFinish();

I don't know which effect has calling waitForExplicitFinish inside a method invoked by registerCleanupFunction, at first glance it seems that it could be working though since we check for .done before really moving on, but please double check that we don't move to the next test till this is really finished with some debug print or wahtever method you prefer.
Attachment #504582 - Flags: review?(mak77) → review+