Last Comment Bug 489259 - Loading indicator stays loading when remove iframe in onload
: Loading indicator stays loading when remove iframe in onload
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-20 16:20 PDT by Jeffrey
Modified: 2011-09-09 12:30 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase Loading indicator iframe bug (767 bytes, text/html)
2011-05-10 20:39 PDT, Jeffrey
no flags Details
This fixes it (17.48 KB, patch)
2011-09-07 19:26 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
The right patch, even (6.10 KB, patch)
2011-09-07 19:28 PDT, Boris Zbarsky [:bz] (still a bit busy)
bzbarsky: review-
Details | Diff | Splinter Review
Fix for real real this time (7.02 KB, patch)
2011-09-07 19:41 PDT, Boris Zbarsky [:bz] (still a bit busy)
jst: review+
Details | Diff | Splinter Review

Description Jeffrey 2009-04-20 16:20:48 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8

When posting a form to an iframe and clearing the parent div with Javascript when the onload function is called, the loading indicator stays displaying. 

Html to reproduce:

<html>
<head>
	<script language="JavaScript">
	var first = true;
	function removeIt() {		
		if (first) {
			first = false;		
		} else {
			document.getElementById('container').innerHTML = "";
		}
	}
	</script>
</head>
<body>
	<div id="container">
		<iframe style="width: 300px; height: 300px;" src="http://www.google.nl" id="iframetest" name="iframetest" onload="removeIt();"></iframe>
	</div>
	<form target="iframetest" method="get" action="http://www.google.nl/search">
		<input type="text" name="q">
		<input type="submit">
	</form>
</body>
</html>

Reproducible: Always

Steps to Reproduce:
1. Create a div, and create an iframe in it
2. Create form element with target to the iframe
3. Put in the onload a javascript code which will clear the parent div.
4. Post the form. 
Actual Results:  
Firefox says it's loading, while it really doesn't nothing.

Expected Results:  
Loading indicator should stop. 

Default settings.
Comment 1 Tyler Downer [:Tyler] 2010-10-06 14:17:15 PDT
This is a mass search for bugs which are in the Firefox General component, are
UNCO, have not been changed for 500 days and have an unspecified version. 

Reporter, can you please update to Firefox 3.6.10 or later, create a fresh profile, http://support.mozilla.com/en-US/kb/managing+profiles, and test again. If you still see the issue, please update this bug. If the issue is gone, please set the status to RESOLVED > WORKSFORME.
Comment 2 Tyler Downer [:Tyler] 2010-11-01 22:40:19 PDT
No reply from reporter, INCOMPLETE. Please retest with Firefox 3.6.12 or later and a new profile (http://support.mozilla.com/kb/Managing+profiles). If you continue to see this issue with the newest firefox and a new profile, then please comment on this bug.
Comment 3 Jeffrey 2010-11-02 00:17:14 PDT
This issue is still there in Firefox 3.6.12 and also on Windows 7. Just copy-paste the source and submit the form to reproduce.
Comment 4 Tyler Downer [:Tyler] 2011-05-10 20:03:40 PDT
Jeffery, would you mind retesting with Firefox 4? there have been some pretty significant changes since 3.6, they may alleviate the issue. If it still happens, can you make a reduced testcase in an HTML file and attach it to this bug? thank you.
Comment 5 Jeffrey 2011-05-10 20:38:12 PDT
Retested it in firefox 4 (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1) and the bug is still there. Tested it with the HTML from the description, but I will include it as attachment now with a small description.
Comment 6 Jeffrey 2011-05-10 20:39:00 PDT
Created attachment 531530 [details]
Testcase Loading indicator iframe bug
Comment 7 Sidney San Martín 2011-09-06 10:54:23 PDT
FWIW, I can reproduce this in Firefox 6.0.2 on Mac.

Libraries that deal with iframes work around this issue by removing the iframe after a delay:
https://github.com/malsup/form/blob/e77e287c8024d200909a7b4f4a1224503814e660/jquery.form.js#L511
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-06 11:39:12 PDT
bz fixed something similar in bug 300849
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-09-07 19:08:13 PDT
The difference between bug 300849 and here is that mIsLoadingDocument is false on the parent loader when ChildDoneWithOnload is called, so DocLoaderIsEmpty is a no-op.

Which is correct as far as it goes, since we do NOT want to be firing onload on the parent loader.

The problem in this bug is that in nsDocLoader::doStopDocumentLoad we fire the STATE_STOP|STATE_IS_DOCUMENT state change, which fires the load event and removes us, then we fire the STATE_STOP|STATE_IS_WINDOW_STATE_IS_NETWORK state change which would normally stop the throbber... but at that point we're been removed from the docshell tree.

The right fix for this is to make all this stuff sane somehow.

The expedient fix is to grab the whole parent chain up front before calling FireOnStateChange and then call it manually on each item in the parent chain.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-09-07 19:26:22 PDT
Created attachment 559028 [details] [diff] [review]
This fixes it

If someone has bright ideas for an automated test, please feel free!
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-09-07 19:28:27 PDT
Created attachment 559029 [details] [diff] [review]
The right patch, even
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-09-07 19:30:45 PDT
Comment on attachment 559029 [details] [diff] [review]
The right patch, even

Er, no.  This is wrong....
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-09-07 19:41:00 PDT
Created attachment 559031 [details] [diff] [review]
Fix for real real this time
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-08 17:29:34 PDT
Comment on attachment 559031 [details] [diff] [review]
Fix for real real this time

+    typedef nsAutoTArray<nsRefPtr<nsDocLoader>, 2> WebProgressList;

Should the 2 be a 3 here to avoid allocation overhead in the not at all uncommon case of chrome->content->iframe?

r=jst either way.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-09-08 18:32:06 PDT
Since these are stack arrays, I'll just make it 8 to be pretty safe.  Good catch!
Comment 16 Sidney San Martín 2011-09-08 20:07:53 PDT
(In reply to Boris Zbarsky (:bz) from comment #15)
> Since these are stack arrays, I'll just make it 8 to be pretty safe.  Good
> catch!

Out of pure, morbid curiosity (as someone not familiar with C++), what would happen if I nested too many iframes on a page and triggered this case?
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-09-08 20:15:42 PDT
Then we'd have to allocate some heap memory instead, once we found that the nesting is more than 8 levels deep.  So that case would be a little slower and maybe use a tiny bit (~100 bytes) more memory.

Note You need to log in before you can comment on or make changes to this bug.