Closed Bug 846458 Opened 12 years ago Closed 12 years ago

intermittent TEST-UNEXPECTED-PASS | /tests/content/base/test/test_bug548193.html | Assertion count 0 is less than expected range 1-1 assertions.

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: dbaron, Assigned: imelven)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

patch v2
5.24 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
I think this is a rare enough intermittent unexpected pass that it's better off filed and starred than annotated (so we know when the assertion is fixed): https://tbpl.mozilla.org/php/getParsedLog.php?id=20190512&tree=Mozilla-Inbound Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitest-1 on 2013-02-28 11:03:02 PST for push 67060725ec8d slave: talos-r4-snow-054 11:08:27 INFO - 33536 ERROR TEST-UNEXPECTED-PASS | /tests/content/base/test/test_bug548193.html | Assertion count 0 is less than expected range 1-1 assertions.
The test examines the upload stream in http-on-modify-request. It needs to rewind the stream before returning or this assertion fires. What I don't understand is how come sometimes this assertion *doesn't* fire.
Assignee: nobody → imelven
Component: DOM → Security
Ian, have you had a chance to look at this yet? This is a top orange at the moment & I'd like to avoid just annotating this away if possible :-)
Flags: needinfo?(imelven)
(In reply to Ed Morley [:edmorley UTC+0] from comment #59) > Ian, have you had a chance to look at this yet? This is a top orange at the > moment & I'd like to avoid just annotating this away if possible :-) Not yet, but I'll take a look today, thanks for the poke :)
Flags: needinfo?(imelven)
Pasting in some discussion from irc about khuey's initial investigation : 12:44 <@khuey> so what's happening is 12:44 <@khuey> the test hooks into http-on-modify-request 12:44 <@khuey> and looks at the upload stream 12:44 <@khuey> and reads from it 12:44 <@khuey> then later necko uploads it 12:45 <@khuey> and since the stream is seeked past 0 it asserts 12:45 <@khuey> that's why the assertion happens 12:45 < jduell> khuey: I'd think it's up to the sniffer to reseek to 0? 12:45 <@khuey> I don't understand why sometimes it *doesn't* happen 12:45 <@khuey> that seems to imply that http-on-modify-request and upload starting are racing ... 12:46 < jduell> khuey: OMR is now called a bit later than it used to be 12:46 < jduell> it used to be called synchronously during asyncOpen 12:46 < jduell> Now it gets called after we've done some proxy resolution. 12:46 < jduell> khuey: We have a new on-opening-request notification that still runs sync within asyncOpen 12:47 <@khuey> ah 12:49 < jduell> khuey: here's a start to the breadcrumb trail if you're interested: https://bugzilla.mozilla.org/show_bug.cgi?id=800799#c14 I don't immediately see a way to rewind the upload stream after reading from it. Maybe if we could copy it and read from the copy ? Alternately, we could switch to the new on-opening-request notification and see if that removes the race and the intermittant-ness - it sounds like it might. Is there a good way to test if that helps ?
Here is a patch that switches to using http-on-opening-request and removes the EnablePrivilege usage (replacing it with SpecialPowers) - this might be more consistent in always asserting about not rewinding the stream. I tried seeking inputStream back to the beginning via QI'ing it to nsISeekableStream, but this then triggered another assertion : ###!!! ASSERTION: OnDataAvailable implementation consumed no data
I know what the OnDataAvailable assertion is about now, it's indeed a bug in the CSP code, patch coming shortly.
Attached patch patch v2 — — Splinter Review
CSPViolationReportListener was ignoring the note on MDN about nsIStreamListener::OnDataAvailable saying that you have to read aCount bytes of data before returning - this patch does that as well as the other stuff from the previous patch. I removed the SimpleTest.expectAssertions(1) and was going to pushed this to try to run m-1 for all platforms debug builds to have a look but the try tree is closed so that will have to wait :)
Attachment #725614 - Attachment is obsolete: true
Attachment #725673 - Flags: review?(sstamm)
Comment on attachment 725673 [details] [diff] [review] patch v2 Review of attachment 725673 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, looks good to me. It's a bummer we have to create an instance of nsIScriptableInputStream instead of just futzing with the existing stream, but I can't see a better way. (Probably when we reimpl this in C++ we can avoid creating another stream obj.)
Attachment #725673 - Flags: review?(sstamm) → review+
https://hg.mozilla.org/mozilla-central/rev/baa90816699d
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: