575377 - Loading forever when open a page which contains multipart image/motion jpeg streams (Webcam)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100628 Minefield/3.7a6pre ID:20100628035917
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100628 Minefield/3.7a6pre ID:20100628035917

Loading forever when open a page which contains multipart image/motion jpeg streams(Webcam).
Progress meter in the statusbar and in the tab indicate not 100% and not disappear.

Tjis problem does not happens on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.7pre) Gecko/20100626 Namoroka/3.6.7pre ID:20100626041846.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open URL ( http://www.upl.cs.wisc.edu/w/Webcam )

Actual Results:
 Progress meter in the statusbar and in the tab indicates not 100%.
 And loading forever.

Expected Results:
 Progress meter in the statusbar and in the tab should indicate 100% and should disappear after the leading completion of the page.

Works(Progress meter disappears after complete page loading, but the image flickers)
http://hg.mozilla.org/mozilla-central/rev/ff3496b1f6c7
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090912 Minefield/3.7a1pre ID:20090912042051

Fails(Loading forever):
http://hg.mozilla.org/mozilla-central/rev/bf0fdec8f43b
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090913 Minefield/3.7a1pre ID:20090913050738

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ff3496b1f6c7&tochange=bf0fdec8f43b

Comment 1

[Alice0775 White]

I guess it is regression of Bug 512435 - document onload should not fire until all visible images have 1 decoded frame

Comment 2

[Måns Nilsson]

No issues for me. The load bar shouldn't stop, because it is a live picture, which basically means it is downloading all the time.

Comment 3

[Alice0775 White]

(In reply to comment #2)
> No issues for me. The load bar shouldn't stop, because it is a live picture,
> which basically means it is downloading all the time.

Namoroka 3.6.7pre and Google Chrome: the progress bar disappears.
And HTML5 video, Flash movie and etc... : the  progress bar also disappears though it is downloading stream data.
So, I think the Webcam sterem should be  also same handling  as other streaming.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, this isn't something we should be shipping...

Comment 5

[Bobby Holley (:bholley)]

Just looked into this. What appears to be happening is that the multipart image fires the cycle of notifications continuously, all of which are received by nsImageLoadingContent. When OnStartDecode() is received, SetBlockingOnload(PR_TRUE) gets called, and the load is blocked. When OnStopFrame() is received, SetBlockingOnload(PR_FALSE) gets called, and the load is unblocked. However, this doesn't seem to have enough time to take effect before the next OnStartDecode() is received, so we never completely finish loading the document.

Disabling the machinery in SetBlockingOnload() fixes this problem.

Bz - does this all make sense? What would you propose as a solution?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> SetBlockingOnload(PR_TRUE) gets called, and the load is blocked. When
> OnStopFrame() is received, SetBlockingOnload(PR_FALSE) gets called, and the
> load is unblocked. 

This all makes sense.

> However, this doesn't seem to have enough time to take effect before the next
> OnStartDecode() is received

Yeah, since the unblock here is async.

> What would you propose as a solution?

Not blocking onload for the latter parts of a multipart image.  We try hard in imagelib to do that (see the LOAD_BACKGROUND stuff in imgRequestProxy).  This means the image loading content needs to be able to tell whether it's looking at such a latter part of a multipart image.

Comment 7

[Bobby Holley (:bholley)]

Attachment: patch v1

Added a patch that seems to do the trick.

Flagged bz for review, and pushed to try as 4840b879ad03.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 455287 [details] [diff] [review]
patch v1

Good catch on the load flag.  LOAD_BACKGROUND requests should never block onload, so we just don't want to block for them, period, no matter how they got to be LOAD_BACKGROUND.  Just say that and nix the part about multipart.

r- because you need to fix OnStopDecode to avoid unbalanced unblocks, no?

Comment 9

[Bobby Holley (:bholley)]

Attachment: patch v2

(In reply to comment #8)
> Just say that and nix the part about multipart.

Done.

> r- because you need to fix OnStopDecode to avoid unbalanced unblocks, no?

I don't think so. SetBlockingOnload(PR_FALSE) only does something if SetBlockingOnload(PR_TRUE) was called previously (this is the reason for the slightly awkward naming). So the rest of those calls should just be no-ops.

This could use some test coverage. We had a bug somewhere about multipart/x-mixed-replace test coverage, but I can't find it. I've emailed joe about it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 455301 [details] [diff] [review]
patch v2

>+  if (NS_SUCCEEDED(rv) && (loadFlags & nsIRequest::LOAD_BACKGROUND))
>+    background = PR_TRUE;

How about:

  PRBool background = 
    (NS_SUCCEEDED(rv) && (loadFlags & nsIRequest::LOAD_BACKGROUND));

>+  // Block onload if it's the current request and not background
>+  if ((aRequest == mCurrentRequest) && !background) {

Please don't overparenthesize the == check.  And maybe do the whole flag-getting stuff only if aRequest == mCurrentRequest, unless there's a good reason to do it in all cases?

r=bzbarsky with those nits.

Comment 11

[Bobby Holley (:bholley)]

Attachment: patch v3

Attached the patch I'm going to push. I'll let it run on try first.

Comment 12

[Bobby Holley (:bholley)]

pushed to try as 33429b3d126a.

Comment 13

[Måns Nilsson]

Please STOP posting here. I have been getting 6 emails TODAY, only to find out all of them where actually from this thread and Mozilla Bugzilla. This is not damn funny at all. PLEASE STOP NOW!

Comment 14

[Måns Nilsson]

No. I NEVER added myself to the cc list of this bug, I believe. It probably happened automatically after I made my first post in this thread. Thanks.

Comment 15

[Bobby Holley (:bholley)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/255acc6edc7b

Resolving fixed.

*Ducks*