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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1 + mixed content tests reenable

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Honza Bambas (:mayhemer)]

(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.

Comment 3

[Honza Bambas (:mayhemer)]

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

[Boris Zbarsky [:bzbarsky]]

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).

Comment 5

[Honza Bambas (:mayhemer)]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Honza Bambas (:mayhemer)]

(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

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2 + mc tests reenable

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.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: interdiff of attachement 362601 and v2

Comment 11

[Boris Zbarsky [: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?

Comment 12

[Honza Bambas (:mayhemer)]

(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.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: interdiff of attachement 362601 and v2.1 (upcoming)

Just an interdiff for now.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> In OnStopRequest we do info->mMaxProgress = info->mCurrentProgress 

Ah, ok.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v2.1

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.

Comment 16

[Honza Bambas (:mayhemer)]

Christian, the base patch is https://bugzilla.mozilla.org/attachment.cgi?id=362601&action=edit.

Comment 17

[Johnny Stenback (:jst)]

bz, do you think you could do the reviews here? Biesi is unlikely to get to this any time soon it seems.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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....

Comment 19

[Johnny Stenback (:jst)]

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

Comment 20

[Honza Bambas (:mayhemer)]

jst: can I take your r+ as a complete review including the interdiff or it still needs bz's review?

Comment 21

[Johnny Stenback (:jst)]

I'd say go ahead and land this, but bz is still welcome review if he wants to.

Comment 22

[Johnny Stenback (:jst)]

Honza, can you get this checked in on trunk and branch now, or are you blocked by something else?

Comment 23

[Honza Bambas (:mayhemer)]

Going to land this, today or tomorrow morning (my local).

Comment 24

[Honza Bambas (:mayhemer)]

Attachment: As landed on mozilla-central [Checkin comment 24]

http://hg.mozilla.org/mozilla-central/rev/96746395df4f

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: As landed on 1.9.1 [Checkin comment 25]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b29668acc485

Comment 26

[Dão Gottwald [:dao]]

This caused test_dynUnsecureRedirect.html to fail on all platforms, and it looks like it didn't bake sufficiently before landing on branch.

Comment 27

[Honza Bambas (:mayhemer)]

(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

[Dão Gottwald [:dao]]

(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.

Comment 29

[Honza Bambas (:mayhemer)]

(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

[Boris Zbarsky [:bzbarsky]]

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!

Comment 31

[Boris Zbarsky [:bzbarsky]]

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).

Comment 32

[Honza Bambas (:mayhemer)]

(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.