Closed Bug 489259 Opened 15 years ago Closed 13 years ago

Loading indicator stays loading when remove iframe in onload

Categories

(Core :: General, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: comgenie, Assigned: bzbarsky)

Details

(Keywords: testcase)

Attachments

(2 files, 2 obsolete files)

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.
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.
Whiteboard: [CLOSEME 2010-11-01]
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.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → INCOMPLETE
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.
Status: RESOLVED → UNCONFIRMED
OS: Windows XP → Windows 7
Resolution: INCOMPLETE → ---
Whiteboard: [CLOSEME 2010-11-01]
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.
Version: unspecified → 3.6 Branch
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.
Keywords: testcase
Version: 3.6 Branch → 4.0 Branch
Product: Firefox → Core
QA Contact: general → general
Version: 4.0 Branch → 2.0 Branch
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
bz fixed something similar in bug 300849
Attachment #531530 - Attachment mime type: text/plain → text/html
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.
Attached patch This fixes it (obsolete) — Splinter Review
If someone has bright ideas for an automated test, please feel free!
Attachment #559028 - Flags: review?(jst)
Attached patch The right patch, even (obsolete) — Splinter Review
Attachment #559029 - Flags: review?(jst)
Attachment #559028 - Attachment is obsolete: true
Attachment #559028 - Flags: review?(jst)
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]
Version: 2.0 Branch → Trunk
Comment on attachment 559029 [details] [diff] [review]
The right patch, even

Er, no.  This is wrong....
Attachment #559029 - Flags: review?(jst) → review-
Attachment #559031 - Flags: review?(jst)
Attachment #559029 - Attachment is obsolete: true
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.
Attachment #559031 - Flags: review?(jst) → review+
Since these are stack arrays, I'll just make it 8 to be pretty safe.  Good catch!
Whiteboard: [need review] → [need landing]
(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?
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.
http://hg.mozilla.org/mozilla-central/rev/b201507b95c7
Status: NEW → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla9
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.