Closed Bug 480713 Opened 11 years ago Closed 11 years ago

nsDocLoader doesn't clear requests from mRequestInfoHash, security UI may not report mixed content

Categories

(Core :: Document Navigation, defect, P2, major)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Keywords: fixed1.9.1)

Attachments

(4 files, 3 obsolete files)

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)
Summary: nsDocLoader doesn't clear requests from mRequestInfoHash → nsDocLoader doesn't clear requests from mRequestInfoHash, security UI may not report mixed content
Whiteboard: [has patch][needs review bzbarsky]
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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-
(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.
Status: NEW → ASSIGNED
Depends on: 329869
Whiteboard: [has patch][needs review bzbarsky]
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?
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).
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).
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.
(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.
> 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?
Attached patch v2 + mc tests reenable (obsolete) — Splinter Review
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)
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-
(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.
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)
> In OnStopRequest we do info->mMaxProgress = info->mCurrentProgress 

Ah, ok.
Attached patch v2.1Splinter Review
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)
bz, do you think you could do the reviews here? Biesi is unlikely to get to this any time soon it seems.
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....
Attachment #366653 - Flags: superreview+
Attachment #366653 - Flags: review?(cbiesinger)
Attachment #366653 - Flags: review+
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
jst: can I take your r+ as a complete review including the interdiff or it still needs bz's review?
I'd say go ahead and land this, but bz is still welcome review if he wants to.
Honza, can you get this checked in on trunk and branch now, or are you blocked by something else?
Going to land this, today or tomorrow morning (my local).
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #371637 - Attachment description: As landed on mozilla-central → As landed on mozilla-central [Checkin comment 24]
Keywords: fixed1.9.1
This caused test_dynUnsecureRedirect.html to fail on all platforms, and it looks like it didn't bake sufficiently before landing on branch.
(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.
(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.
(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 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+
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).
(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.
Depends on: 487915
You need to log in before you can comment on or make changes to this bug.