Closed Bug 648485 Opened 14 years ago Closed 14 years ago

Intermittent failure in content/base/test/test_bug482935.html | Received fresh value for second request - got "1", expected "0"

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla6
Tracking Flags:
Tracking Status
firefox5 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: bjarne)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Using asynchronous xhrs instead of synchronous
1.58 KB, patch
ehsan.akhgari
: review+
sayrer
: approval-mozilla-aurora+
Details | Diff | Splinter Review
http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1302227102.1302229749.31490.gz Rev3 MacOSX Leopard 10.5.8 cedar debug test mochitests-1/5 on 2011/04/07 18:45:02 25764 INFO TEST-START | /tests/content/base/test/test_bug482935.html ++DOMWINDOW == 17 (0x1c916148) [serial = 540] [outer = 0x1c155da0] 25765 INFO TEST-PASS | /tests/content/base/test/test_bug482935.html | Expected empty response to cancelled request - "" should equal "" 25766 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug482935.html | Received fresh value for second request - got "1", expected "0" 25767 INFO TEST-PASS | /tests/content/base/test/test_bug482935.html | Expected empty response to cancelled request - "" should equal "" 25768 INFO TEST-PASS | /tests/content/base/test/test_bug482935.html | Received cached value for second request - "1" should equal "1" 25769 INFO TEST-END | /tests/content/base/test/test_bug482935.html | finished in 2092ms
OS: Mac OS X → All
Hardware: x86 → All
That might fix it. At least, it's not broken locally.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #524696 - Flags: review?(ehsan)
Whiteboard: [orange] → [orange][has patch][needs review]
Attachment #524696 - Flags: review?(ehsan) → review+
Pushed: http://hg.mozilla.org/mozilla-central/rev/6f1a2405f71e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [orange][has patch][needs review] → [orange]
Target Milestone: --- → mozilla2.2
Attachment #524696 - Attachment description: Changes setTimeout to executeSoon → Changes setTimeout to executeSoon [checked-in]
The push right after that says "nope, not quite fixed."
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm, seems like this is now more flaky... :/
Attached patch Second attempt [checked-in] (obsolete) — — Splinter Review
This patch should be better.
Attachment #524696 - Attachment is obsolete: true
Attachment #525310 - Flags: review?(ehsan)
Comment on attachment 525310 [details] [diff] [review] Second attempt [checked-in] Yeah, this does look better. But it's best to pass the second parameter to setTimeout explicitly (0). r=me with that fixed.
Attachment #525310 - Flags: review?(ehsan) → review+
Pushed the second attempt: http://hg.mozilla.org/mozilla-central/rev/7a0810457ecd
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Attachment #525310 - Attachment description: Second attempt → Second attempt [checked-in]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I have no clue why this doesn't work. Bjarne and Jason, do you have an idea by any chance?
Blocks: 482935
(In reply to comment #16) > I have no clue why this doesn't work. Bjarne and Jason, do you have an idea by > any chance? Please see bug #482935, comment #69 and please also observe the time between landing bug #482935 (March 25th) and the first report of this failure (April 7th).
(In reply to comment #22) > please also observe the time between > landing bug #482935 (March 25th) and the first report of this failure (April > 7th). Do you have another changeset in mind which might have caused this failure?
Group: mozilla-confidential
Group: mozilla-confidential
(In reply to comment #23) > Do you have another changeset in mind which might have caused this failure? Not really - I'm just pointing out the timespan and the fact that there was another unrelated issue (bug #552651 I believe) which caused this *test* to always leak independent of the code-change from bug #482935. Btw, is it just me or has the frequency of this increased? (8 reports on April 19th vs 2 reports on April 12th)
(In reply to comment #55) > Btw, is it just me or has the frequency of this increased? (8 reports on April > 19th vs 2 reports on April 12th) My fault. While trying to fix it, I actually make it more orange. Though, I don't think we should backout unless my patches went in the wrong direction. Bjarne, do you think you can find the time to investigate? To know if I should take some to do it.
(In reply to comment #56) > Bjarne, do you think you can find the time to investigate? To know if I should > take some to do it. And you seem to be the best person to look at that ;)
(In reply to comment #57) > And you seem to be the best person to look at that ;) Who said that flattery will get you nowhere..? ;) Try changing the two xhr2.open-calls to do async requests (i.e. remove last param). Since you've changed them to terminate on listeners anyway there's no reason to do those calls synchronously. I tried it locally and it works (but I'm not able to reproduce the error). I could submit a patch but this change is utterly trivial and you seem to have a shorter review-approval-checkin path than I do... Why would this improve the situation? Well, I'm actually not 100% sure it will but it's worth a shot and it won't hurt. XHR spins the main-thread event-loop on sync-requests and that has never seemed to me like a very safe thing to do.
(In reply to comment #60) > (In reply to comment #57) > > And you seem to be the best person to look at that ;) > > Who said that flattery will get you nowhere..? ;) Try changing the two > xhr2.open-calls to do async requests (i.e. remove last param). Since you've > changed them to terminate on listeners anyway there's no reason to do those > calls synchronously. I tried it locally and it works (but I'm not able to > reproduce the error). That was no flattery, you obviously know much more than I do about what the test is actually testing. > I could submit a patch but this change is utterly trivial > and you seem to have a shorter review-approval-checkin path than I do... You just have to know the person to ask the review to ;). If you write a patch and ask a review to Ehsan, I can bet he will reply *very* quickly: he is very responsive for random oranges patches. > Why would this improve the situation? Well, I'm actually not 100% sure it will > but it's worth a shot and it won't hurt. XHR spins the main-thread event-loop > on sync-requests and that has never seemed to me like a very safe thing to do. The only thing I would not like to happen is to hide a real issue by writing a test that is going around it.
(In reply to comment #61) > The only thing I would not like to happen is to hide a real issue by writing a > test that is going around it. It won't hide anything - it will indicate that spinning the event-loop causes issues.
Attachment #525310 - Attachment is obsolete: true
Attachment #527243 - Flags: review?(ehsan)
Comment on attachment 527243 [details] [diff] [review] Using asynchronous xhrs instead of synchronous Cool, let's try this!
Attachment #527243 - Flags: review?(ehsan) → review+
Ok - requesting checkin...
Keywords: checkin-needed
Fixed in cedar. Should be synced with mozilla-central in the next 24 hours.
Assignee: mounir.lamouri → bjarne
Keywords: checkin-needed
Whiteboard: [orange] → [orange][has patch][fixed in cedar]
Target Milestone: mozilla5 → mozilla6
Pushed: http://hg.mozilla.org/mozilla-central/rev/4230d1472963 And thanks for the patch Bjarne :)
Flags: in-testsuite+
Whiteboard: [orange][has patch][fixed in cedar] → [orange]
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Thank you all! :-)
Test only change for a random orange. Can we land it on aurora?
tracking-firefox5: --- → ?
Attachment #527243 - Flags: approval-mozilla-aurora+
Please land for Aurora by Monday May 16 or the approval will potentially be lost. Please mark as status-firefox5 fixed when you do.
I'm really not sure about the procedure to get this into aurora so I'll try first with good, old "checkin-needed" (assuming I'm supposed to drive this)...
Keywords: checkin-needed
(In reply to comment #85) > Test only change for a random orange. Can we land it on aurora? You can generally do this without approval.
Whiteboard: [orange] → [orange] [needs aurora landing]
Whiteboard: [orange]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: