Closed Bug 886054 Opened 7 years ago Closed 6 years ago

intermittent test_bookmarks_json.js | Test timed out

Categories

(Toolkit :: Places, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox24 --- disabled
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 --- disabled

People

(Reporter: philor, Assigned: raymondlee)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 4 obsolete files)

https://tbpl.mozilla.org/php/getParsedLog.php?id=24473144&tree=Mozilla-Inbound
Ubuntu VM 12.04 x64 mozilla-inbound pgo test xpcshell on 2013-06-22 11:35:54 PDT for push 6e0f29d574e4
slave: tst-linux64-ec2-033

11:57:04  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_bookmarks_json.js | Test timed out
11:57:04  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_bookmarks_json.js | test failed (with xpcshell return code: -6), see following log:
11:57:04     INFO -  >>>>>>>
...
11:57:04     INFO -  <<<<<<<

Looks rather like a bogus timeout when it didn't actually time out.
Andres and/or Raymond, I see that you two have worked on this test. Can you one you please look into this?
Flags: needinfo?(raymond)
Flags: needinfo?(andres)
I will look into it.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Flags: needinfo?(raymond)
Flags: needinfo?(andres)
Test disabled on OSX on beta for Fx24. We can always re-enable it if a fix is found.
https://hg.mozilla.org/releases/mozilla-beta/rev/2e95b1b0f005

Note that test_bookmarks_html.js is also disabled on OSX on beta for intermittent failures in bug 869802.
Attached patch v1 (obsolete) — Splinter Review
The timeout happened when the case "icon" in checkItem() doesn't return anything.  Therefore, I have changed the code to ensure that the {Promise} is returned before  doing the checks.  The same issue happens in test_bookmarks_html.js for bug 869802 so this patch should also fix that.
Attachment #802035 - Flags: review?(mano)
Mano, review ping?
Flags: needinfo?(mano)
Comment on attachment 802035 [details] [diff] [review]
v1


>+  let deferred;
...
>+          deferred = Promise.defer();

I think you could use the let statement for cleaner code.

let (deferred = ....) {
}

Or just wait one day so I land https://bugzilla.mozilla.org/show_bug.cgi?id=896193 and use the new API (it won't break your patch because the callbacks api stays for now, but I think your patch could benefit).
Attachment #802035 - Flags: review?(mano) → feedback+
If this fix is made to depend on bug 896193, does that mean it won't be suitable for uplift to the release branches?
Attached patch v2 (obsolete) — Splinter Review
(In reply to Mano from comment #36)
> Comment on attachment 802035 [details] [diff] [review]
> v1
> 
> 
> >+  let deferred;
> ...
> >+          deferred = Promise.defer();
> 
> I think you could use the let statement for cleaner code.
> 
> let (deferred = ....) {
> }
> 

I've taken this approach and updated the patch since there is a concern in comment 37

> Or just wait one day so I land
> https://bugzilla.mozilla.org/show_bug.cgi?id=896193 and use the new API (it
> won't break your patch because the callbacks api stays for now, but I think
> your patch could benefit).
Attachment #815236 - Flags: review?(mano)
Comment on attachment 815236 [details] [diff] [review]
v2

data also shouldn't be declared like this. I'd just inline it.
Attachment #815236 - Flags: review?(mano) → review+
So, for trunk, please do wait for the patch from bug 896193. It shouldn't be hard to update the patch.

For any other branch, this patch is fine as it is.
(In reply to Mano from comment #39)
> Comment on attachment 815236 [details] [diff] [review]
> v2
> 
> data also shouldn't be declared like this. I'd just inline it.

Done

(In reply to Mano from comment #40)
> So, for trunk, please do wait for the patch from bug 896193. It shouldn't be
> hard to update the patch.
> 
> For any other branch, this patch is fine as it is.

Pushed to try and waiting for results.
https://tbpl.mozilla.org/?tree=Try&rev=e19819d87b3c
Attachment #802035 - Attachment is obsolete: true
Attachment #815236 - Attachment is obsolete: true
(In reply to Raymond Lee [:raymondlee] from comment #41)
> Pushed to try and waiting for results.
> https://tbpl.mozilla.org/?tree=Try&rev=e19819d87b3c

Passed try but don't land on trunk as mentioned in comment 41
Keywords: checkin-needed
Pushed to Aurora/Beta with the commit message tweaked (in general, the commit message should be saying what the patch is actually doing, not restating the bug title). We'll see how it goes :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/c186283ffd5c
https://hg.mozilla.org/releases/mozilla-beta/rev/33970ef508b0
Depends on: 896193
Attached patch v4 for trunk (obsolete) — Splinter Review
This patch is based on the patch in bug 896193 for trunk.
Attachment #817657 - Flags: review?(mano)
(In reply to Raymond Lee [:raymondlee] from comment #44)
> Created attachment 817657 [details] [diff] [review]
> v4 for trunk
> 
> This patch is based on the patch in bug 896193 for trunk.

Mano: review ping
Comment on attachment 817657 [details] [diff] [review]
v4 for trunk

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

I'm stealing this review, off-hand the fix looks ok, even if I don't get the try/catch around getLivemark calls, so I must verify if those are really needed.
Attachment #817657 - Flags: review?(mano) → review?(mak77)
Attached patch v5 for trunkSplinter Review
This is my review already applied to the patch, the patch was correct, but as I thought the try/catch were not really needed.
Attachment #817657 - Attachment is obsolete: true
Attachment #817657 - Flags: review?(mak77)
Attachment #8366838 - Flags: review+
Flags: in-testsuite+
Keywords: checkin-needed
Attachment #815250 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d3f107710885
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
https://hg.mozilla.org/releases/mozilla-aurora/rev/d20c6ac43fcd

(This was the non-trunk patch that was previously landed on other release branches)
Duplicate of this bug: 869802
You need to log in before you can comment on or make changes to this bug.