Closed
Bug 480713
Opened 16 years ago
Closed 16 years ago
nsDocLoader doesn't clear requests from mRequestInfoHash, security UI may not report mixed content
Categories
(Core :: DOM: Navigation, defect, P2)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
(Keywords: fixed1.9.1)
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
This is cause for mixed content tests (bug 452401) intermittent failures.
nsDocLoader maps request info by request (nsIRequest) address in its mRequestInfoHash hash table. When a request address get recycled and reused for another request (not unlikely to happen), existing info for already dead request is reused.
In nsDocLoader::OnProgress there is a condition 'if (!info->mUploading && (nsInt64(0) == info->mCurrentProgress) && (nsInt64(0) == info->mMaxProgress))' that have to pass only when we get the first OnProgress notify from a request (i.e. when progress of a request is 0) to prevent multiple StateChange notifications to upper levels.
When existing request info is reused the condition doesn't pass even ones.
This OnProgress notification is critical for the security UI. When it doesn't get it for a request, that request got ignored by security UI and we may occasionally get fully secure state for actual mixed content page.
Solution is to remove the request info from the hash table when we get OnStopRequest.
Flags: blocking1.9.1?
Attachment #364674 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•16 years ago
|
Summary: nsDocLoader doesn't clear requests from mRequestInfoHash → nsDocLoader doesn't clear requests from mRequestInfoHash, security UI may not report mixed content
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][needs review bzbarsky]
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment 1•16 years ago
|
||
Comment on attachment 364674 [details] [diff] [review]
v1 + mixed content tests reenable
Sadly, this would break CalculateMaxProgress. This does seem like the right general approach, but but you need to fix the progress stuff.
Note that this patch would conflict with the one I put in bug 329869; it's probably a good idea to get that one in first before messing with this stuff.
Attachment #364674 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 2•16 years ago
|
||
(In reply to comment #1)
> (From update of attachment 364674 [details] [diff] [review])
> Note that this patch would conflict with the one I put in bug 329869; it's
> probably a good idea to get that one in first before messing with this stuff.
I'll wait for that bug get fixed.
Assignee | ||
Comment 3•16 years ago
|
||
Hmm... it seems we cannot wait for that bug, it is not blocking 1.9.1 but this one is. Also to land that bug we should to fix leak caused by its test; so far, I have no idea what it is, could be complicated.
I'd say we have to fix this bug first. Boris, do you agree?
Comment 4•16 years ago
|
||
I thought that bug was blocking too; if not, then we should indeed fix this one first. But maybe we should just take that patch as part of this bug, then, as long as you're more or less rewriting the mRequestInfoHash code (which is what I suspect you'll end up doing here).
Assignee | ||
Comment 5•16 years ago
|
||
Boris, have you considered we could store the request info object using nsIWritablePropertyBag2::setPropertyAsInterface() on channel that support it? Using an instance pointer as a hash key is commonly a bad idea.
There are some that don't: GeckoProtocolChannel, nsWyciwygChannel, nsJSChannel (just read-only), nsExtProtocolChannel, nsPartChannel, nsViewSourceChannel, nsIconChannel(s).
Comment 6•16 years ago
|
||
I'm not sure what you're asking. We need to have request info for already-completed requests available right now to properly calculate max progress. How would storing anything on the requests help that?
> Using an instance pointer as a hash key is commonly a bad idea.
Not if you restrict the hash entries to the lifetime of the object!
> There are some that don't
Every single extension-implemented channel. You can't rely on this being supported, period.
Assignee | ||
Comment 7•16 years ago
|
||
(In reply to comment #6)
> I'm not sure what you're asking. We need to have request info for
> already-completed requests available right now to properly calculate max
> progress. How would storing anything on the requests help that?
>
> > Using an instance pointer as a hash key is commonly a bad idea.
>
> Not if you restrict the hash entries to the lifetime of the object!
>
What might be quit difficult and what we apparently don't here, in this case, (and not only this one). You need to keep all request infos for a lifetime of complete document load, but as I found out, http or what ever channels might die and their address be reused for sub-loads originated by the same document. Keeping a strong ref by the hash table would solve it, but might cause leaks. So, my idea was to attach the info to the request it self, where it actually belongs, but there is not a straight way to do it, as you pointed out bellow, OK.
So, I would personally move the info from the hashtable to some other array on every OnStopRequest and compute the max progress from infos in the current hash table + in the new array that we would both clear (as you do it in you patch for bug 329869) on completion of the document load.
> > There are some that don't
>
> Every single extension-implemented channel. You can't rely on this being
> supported, period.
Comment 8•16 years ago
|
||
> So, I would personally move the info from the hashtable to some other array on
> every OnStopRequest and compute the max progress from infos in the current hash
> table + in the new array that we would both clear (as you do it in you patch
> for bug 329869) on completion of the document load.
Yeah, that sounds like a good plan. Note that we don't need the info necessarily; just the max progress information, right?
Assignee | ||
Comment 9•16 years ago
|
||
This is based on bz's patch from bug 329869, so I'm not sure he should review it :)
Explanation will be better visible in an interdiff I am about to attach at a moment.
Attachment #364674 -
Attachment is obsolete: true
Attachment #365955 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•16 years ago
|
||
Comment 11•16 years ago
|
||
Comment on attachment 365956 [details] [diff] [review]
interdiff of attachement 362601 and v2
I should probably review the interdiff while someone else reviews the base patch.. not sure who. Maybe biesi?
In any case, this looks like it'll behave incorrectly when info->mMaxProgress is -1 or < its final self progress. In both cases we need to set the max progress to -1, no?
Oh, and are you sure the RemoveRequestInfo() needs to happen after the DocLoaderIsEmpty() call and not before it?
Attachment #365956 -
Flags: review-
Assignee | ||
Comment 12•16 years ago
|
||
(In reply to comment #11)
> In any case, this looks like it'll behave incorrectly when info->mMaxProgress
> is -1 or < its final self progress. In both cases we need to set the max
> progress to -1, no?
>
In OnStopRequest we do info->mMaxProgress = info->mCurrentProgress and as I see the code info->mCurrentProgress will never be less then zero, correct me please if I'm wrong, but according to http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIProgressEventSink.idl#76 progress is never less then zero (only place where we assign to it). When re-counting the total progress (CalculateMaxProgress), 'max' local var is drop to -1 on the first request whom maxProgress in unknown. However, deeper look discovered a different bug in my patch: the current request maxProgress is counted twice.
> Oh, and are you sure the RemoveRequestInfo() needs to happen after the
> DocLoaderIsEmpty() call and not before it?
It's better to delete it before, fixing.
Assignee | ||
Comment 13•16 years ago
|
||
Just an interdiff for now.
Attachment #365955 -
Attachment is obsolete: true
Attachment #365956 -
Attachment is obsolete: true
Attachment #365991 -
Flags: review?(bzbarsky)
Attachment #365955 -
Flags: review?(bzbarsky)
Comment 14•16 years ago
|
||
> In OnStopRequest we do info->mMaxProgress = info->mCurrentProgress
Ah, ok.
Assignee | ||
Comment 15•16 years ago
|
||
This is the patch from bug 329869 with attachment 365991 [details] [diff] [review] - the patch for this bug - applied over it. Christian, this r? request is more just for the base patch from bug 329869. The additional patch will review bz.
Attachment #366653 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 16•16 years ago
|
||
Christian, the base patch is https://bugzilla.mozilla.org/attachment.cgi?id=362601&action=edit.
Comment 17•16 years ago
|
||
bz, do you think you could do the reviews here? Biesi is unlikely to get to this any time soon it seems.
Comment 18•16 years ago
|
||
I can do the review that I've been asked for. The other review request is for code that I wrote, so I can't very well review it myself....
Updated•16 years ago
|
Attachment #366653 -
Flags: superreview+
Attachment #366653 -
Flags: review?(cbiesinger)
Attachment #366653 -
Flags: review+
Comment 19•16 years ago
|
||
Comment on attachment 366653 [details] [diff] [review]
v2.1
I'm going to go ahead and take this review from biesi. I spent a bunch of time looking through this, and it looks good to me. The only thing to note was the use of LL_ macros, whihc are no longer needed, but their use match surrounding code, so I'm fine keeping the code consistent. r+sr=jst
Assignee | ||
Comment 20•16 years ago
|
||
jst: can I take your r+ as a complete review including the interdiff or it still needs bz's review?
Comment 21•16 years ago
|
||
I'd say go ahead and land this, but bz is still welcome review if he wants to.
Comment 22•16 years ago
|
||
Honza, can you get this checked in on trunk and branch now, or are you blocked by something else?
Assignee | ||
Comment 23•16 years ago
|
||
Going to land this, today or tomorrow morning (my local).
Assignee | ||
Comment 24•16 years ago
|
||
Attachment #371637 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #371637 -
Attachment description: As landed on mozilla-central → As landed on mozilla-central [Checkin comment 24]
Assignee | ||
Comment 25•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 26•16 years ago
|
||
This caused test_dynUnsecureRedirect.html to fail on all platforms, and it looks like it didn't bake sufficiently before landing on branch.
Assignee | ||
Comment 27•16 years ago
|
||
(In reply to comment #26)
> it looks like it didn't bake sufficiently before landing on branch.
On trunk I re-enabled secure mixed content tests again (this bug was blocking for that). That caused some problems in the past and still causes now, I had to disable the test_dynEnsecureRedirect.html almost immediately again.
On branch there are no such tests, just the code change.
If there is a problem with it, please contact me or feel free to backout from the branch. I am still watching the tree at the moment.
Comment 28•16 years ago
|
||
(In reply to comment #27)
> (In reply to comment #26)
> > it looks like it didn't bake sufficiently before landing on branch.
>
> On trunk I re-enabled secure mixed content tests again (this bug was blocking
> for that). That caused some problems in the past and still causes now, I had to
> disable the test_dynEnsecureRedirect.html almost immediately again.
Sounds OK. The message for the commit that disabled test_dynEnsecureRedirect.html just didn't make that clear.
Assignee | ||
Comment 29•16 years ago
|
||
(In reply to comment #28)
> Sounds OK. The message for the commit that disabled
> test_dynEnsecureRedirect.html just didn't make that clear.
Sorry, experience for next time.
Comment 30•16 years ago
|
||
Comment on attachment 365991 [details] [diff] [review]
interdiff of attachement 362601 and v2.1 (upcoming)
r=bzbarsky too. Sorry this took so long; I was expecting the patch to be much more complicated than this!
Attachment #365991 -
Flags: review?(bzbarsky) → review+
Comment 31•16 years ago
|
||
This just failed on tinderbox:
*** 74531 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_unsecureIframeMetaRedirect.html | FAILURE: insecure meta-tag <iframe> load breaks security, expected broken got secure
*** 74532 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_unsecureIframeMetaRedirect.html | FAILURE: for 'broken' expected flags [0,1,0], insecure meta-tag <iframe> load breaks security
74531 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_unsecureIframeMetaRedirect.html | FAILURE: insecure meta-tag <iframe> load breaks security, expected broken got secure
74532 ERROR TEST-UNEXPECTED-FAIL | /tests/security/ssl/mixedcontent/test_unsecureIframeMetaRedirect.html | FAILURE: for 'broken' expected flags [0,1,0], insecure meta-tag <iframe> load breaks security
make: *** [mochitest-plain] Error 1
Unknown Error: command finished with exit code: 2
(see Windows Tunit machine testing revision bbb30d93a82f on m-c).
Assignee | ||
Comment 32•16 years ago
|
||
(In reply to comment #31)
> This just failed on tinderbox:
>
> *** 74531 ERROR TEST-UNEXPECTED-FAIL |
> /tests/security/ssl/mixedcontent/test_unsecureIframeMetaRedirect.html |
> FAILURE: insecure meta-tag <iframe> load breaks security, expected broken got
> secure
Filled bug 487632 on it.
You need to log in
before you can comment on or make changes to this bug.
Description
•