148981 - STATE_TRANSFERRING not received for external stylesheets

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Kai Engert (:KaiE:)]

Open http://www.kuix.de/misc/test17/index2.php
The page refers to two external documents, index2.js and index2.css.

Expected behaviour:
Web Progress listeners receive OnStateChange events with STATE_TRANSFERRING for
each external document.

Actual behaviour:
Only for index2.js the STATE_TRANSFERRING event is received.
Not for index2.css

Comment 1

[rpotts (gone)]

I bet that external stylesheets doen't set up the proper notificationCallbacks...

i'll take a look.

-- rick

Comment 2

[scottputterman]

Getting on the Mach V radar.

Comment 3

[Jaime Rodriguez, Jr.]

rick - what are the chances we are gonna have a patch for this before monday?

Comment 4

[rpotts (gone)]

Attachment: Synthesize STATE_TRANSFERRING notifications if necessary

Comment 5

[rpotts (gone)]

hey Kai,

here's a patch to make sure that STATE_TRANSFERRING notifications are fired
*even* if the nsIProgressEventSink notifications are not received...

after talking with you about the possibility of 'failed' requests STILL
delivering data... i added some (nasty) code to still fire the TRANSFERRING
notification for failed HTTP channels (if they were able to connect to the
server)...

let me know if you think this code makes sense... or if ONLY firing the
TRANSFERRING notification when the request succeeds is enough...

-- rick

Comment 6

[Kai Engert (:KaiE:)]

I tried this patch, it works great!

Here is a testcase:
  https://www.kuix.de/misc/test17/index3.php
This is a secure page, but loads an insecure stylesheet.

With neither 140836 nor 148981 applied (current state of branch/trunk) go to
that URL. You will see the alert that you are viewing a page with mixed
encryption, as expected.

With having the patch from 140836 applied, we no longer see the mixed encryption
warning, but the page is incorrectly reported as secure.

With having both 140836 and 148981 applied, we again see the mixed encryption
alert, the required behaviour.


Rick, your patch is even better what I originally suggested. Instead of only
fixing stylesheets, your fix will help for any content. This makes a lot of
sense to my, and helps improve the lock icon reliability.

I looked at your patch. I think your additional check makes sense, and I'd
prefer the patch as you attached it. I think it is good to assume that data has
been transfered, even when only the initial server connection was made. This
won't hurt us, because when we have arrived in that state, the SSL handshake
will already have been completed, and we will be able to obtain the security
state for this channel, so we will make the correct decision. When in doubt, I
prefer receiving the TRANSFERRING more often than less.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 88440 [details] [diff] [review]
Synthesize STATE_TRANSFERRING notifications if necessary

r=kaie

The logic in this patch makes sense to me.
(I understand everything but one line, which is:
  if ((oldMax == 0) && (info->mCurrentProgress == 0)) {
because I don't know the DocLoader internals.)

Comment 8

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on adt's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this is asap, then add the "fixed1.0.1" keyword.

Comment 9

[rpotts (gone)]

hey kai,

i added some more comments above the check of oldMax to clarify what's happening.

      //
      // Determine whether a STATE_TRANSFERRING notification should be 
      // 'synthesized'.
      //
      // If nsRequestInfo::mMaxProgress (as stored in oldMax) and
      // nsRequestInfo::mCurrentProgress are both 0, then the
      // STATE_TRANSFERRING notification has not been fired yet...
      //

-- rick

Comment 10

[Darin Fisher]

Comment on attachment 88440 [details] [diff] [review]
Synthesize STATE_TRANSFERRING notifications if necessary

sr=darin

would be great to look into monitoring OnDataAvailable events from
nsDocumentOpenInfo if possible (as i mentioned to rick over the phone), but
perhaps that isn't possible... at any rate, this seems like a reasonable "hack"
for the short term.

Comment 11

[rpotts (gone)]

I realized that hooking into the OnDataAvailable for the DoumentLoadInfo will
only work for top level documents...

Images, stylesheets, javascript, etc do NOT got through the URILoader :-(  So, i
think that for now, this is probably the best alternative...

-- rick

Comment 12

[rpotts (gone)]

i've checked the patch into the trunk...

Comment 13

[Darin Fisher]

rick: of course!! that makes sense. maybe a state attribute is really the way to
go. i smell nsIChannel2 :(

Comment 14

[John Unruh]

Marking fixed on trunk.

Comment 15

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 16

[Kai Engert (:KaiE:)]

Checked in for Rick to 1_0 branch.

Comment 17

[Tom Everingham]

verified trunk, verified1.0.1

Comment 18

[Tom Everingham]

removing fixed1.0.1 keyword