Last Comment Bug 254144 - Iframe reloads when moved around the DOM tree.
: Iframe reloads when moved around the DOM tree.
Status: RESOLVED INVALID
: testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 35 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Andrew Overholt [:overholt]
Mentors:
: 343235 357700 361853 400284 (view as bug list)
Depends on:
Blocks: 276824 361853
  Show dependency treegraph
 
Reported: 2004-08-03 06:55 PDT by Doug Copi
Modified: 2014-12-30 08:40 PST (History)
52 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase which demonstrates this bug (510 bytes, text/html)
2004-08-05 07:07 PDT, Doug Copi
no flags Details
testcase which correctly demonstrates this bug (499 bytes, text/html)
2004-08-05 07:16 PDT, Doug Copi
no flags Details
simple testcase that manipulates DOM inside iframe (396 bytes, text/html)
2006-01-26 23:26 PST, Jesse Ruderman
no flags Details
Demonstration of this bug using frame elements. (341 bytes, text/html)
2006-04-12 08:07 PDT, Doug Copi
no flags Details
Patch proposal V1 (1.76 KB, patch)
2008-02-29 12:22 PST, Mike Moening
jst: review-
Details | Diff | Splinter Review
simple testcase that manipulates DOM inside iframe V2 (967 bytes, text/html)
2008-03-04 19:55 PST, Mike Moening
no flags Details
Patch proposal V2 (8.52 KB, patch)
2008-03-04 20:10 PST, Mike Moening
bugs: review-
Details | Diff | Splinter Review
simple testcase that manipulates DOM inside iframe (401 bytes, text/html)
2008-03-07 13:15 PST, Mike Moening
no flags Details
Final Patch (7.47 KB, patch)
2008-03-07 13:25 PST, Mike Moening
bugs: review-
Details | Diff | Splinter Review
mochitest for this bug (2.46 KB, text/html)
2008-03-07 13:30 PST, Mike Moening
no flags Details
TC 254144.htm (one of the permutations, still asserts the behaviour this bug wanted to implement is correct) (837 bytes, text/html)
2014-10-07 05:32 PDT, Hallvord R. M. Steen [:hallvors]
no flags Details

Description Doug Copi 2004-08-03 06:55:47 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0

Whenever an iframe node is pulled from the DOM tree, the contentWindow is set to
null, forcing a reload of the iframe's content when it is placed back into the
DOM tree.  The content of the iframe is reloaded even if the iframe is placed
back into the same location it was pulled from.

Reproducible: Always
Steps to Reproduce:
1. Go to http://home.earthlink.net/~dcopi/bug/swap_iframes.html.  You will see
Google load in the top iframe and Yahoo in the bottom iframe.
2. Type a string into the Google search text box inside the top iframe. (Don't
press the "Google Search" button) 
3. Click the link labeled "Swap iframes".

Actual Results:  
The string that was typed into the google search box was lost because the
content of the iframes that were switched are reloded.

Expected Results:  
The content of the iframes should remain unchanged and the string that I typed
into the search text box should have persisted during the moveing of the iframe
nodes.

This incorrect behavior can be observed any time an iframe node is pulled from
the DOM tree and re-inserted, whether it is done via an appendChild (see URL
example), a removeChild and then appendChild, or a replaceChild.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2004-08-03 23:16:12 PDT
Please attach the testcase to this bug.

One core problem here is that windows/docshells don't do so well at being
removed from the window/docshell tree...

Another one is that moving the iframe in the DOM can change its src (which
should arguably reload it).
Comment 2 Doug Copi 2004-08-05 07:07:40 PDT
Created attachment 155275 [details]
testcase which demonstrates this bug
Comment 3 Doug Copi 2004-08-05 07:16:17 PDT
Created attachment 155279 [details]
testcase which correctly demonstrates this bug

Original testcase contained invalid URL's
Comment 4 Jesiah S 2004-10-26 14:16:05 PDT
I see this on LInux 2004102606
Comment 5 Graham Houston 2005-01-13 12:17:58 PST
Yes as pointed out appendChild in the spec does a remove of the node from the 
tree first as the node can only have one parentNode... now the case is, is this 
correct behaviuor or is what we're reading as remove mean something else other 
that removeNode.. I would say it does... it simply means remove the node but 
not with a removeNode as this destroys the object... we just do a remove of the 
node as indicated so that we dont have a clone of the node attached in the 
original poisition.. when we do a removeNode as mozilla does it of course 
destroys the object then the appendChild creates it from the original objects 
attributes.. (incorrect).. the spec that I've read unless im reading the wrong 
spec just says "remove node first" NOT removeChild.



Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2005-01-13 12:28:26 PST
> it simply means remove the node but not with a removeNode as this destroys the
> object

removeChild doesn't destroy the object, in general.  I'm not sure what gave you
the idea that it does.

And yes, the removal should be done with removeChild, with all attendant DOM
events firing, etc, etc.  Either that, or done with a function that is an exact
copy of removeChild, which is the same result.
Comment 7 Erik Arvidsson 2005-10-12 17:31:18 PDT
If the iframe is moved from a document with a different xml:base then the src
changes. Should this cause a reload? What if the user has navigated to another
page inside the iframe?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2005-10-13 20:38:09 PDT
> If the iframe is moved from a document with a different xml:base then the src
> changes. Should this cause a reload?

I would say no.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2005-10-13 20:39:22 PDT
Although that's debatable -- for an image we would reload.

It's not clear how the behavior should interact with user navigation, yes. 
Something to think about once there is a topic of discussion (that is, once
iframes don't get blown away on removal from the tree).
Comment 10 Kees van Ginkel 2005-11-09 07:36:48 PST
Confirmed in Firefox 1.5 RC1 on windows. very easy to reproduce
Comment 11 Jesse Ruderman 2006-01-26 23:13:26 PST
Confirmed with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060126 Firefox/1.6a1
Comment 12 Jesse Ruderman 2006-01-26 23:26:41 PST
Created attachment 209807 [details]
simple testcase that manipulates DOM inside iframe
Comment 13 Alex Firshein 2006-02-22 14:04:53 PST
The key difference lies between removing an IFRAME from the DOM (and then adding it back), versus simply moving an IFRAME within the DOM.

If it is removed, then adding it back in of course should reload it in its initial state.

If it is just moved to a different position in the DOM, it should retain its current state, including history and navigation.

The problem though, if I am not mistaken, is that there is no way to move an IFRAME without removing it, as both appendChild and insertBefore inherently function on existing nodes by removing them first and then adding them elsewhere afterwards.
Comment 14 Jesse Ruderman 2006-02-22 14:07:32 PST
> If it is removed, then adding it back in of course should reload it in its
> initial state.

Why?

Comment 15 Alex Firshein 2006-02-22 14:15:46 PST
(In reply to comment #14)
> Why?

Actually yes this is a good point.  Even if it was removed first, the only way it could be added back in later is if a reference to it still existed.  And if this is so, then it should be added in its last current state.
Comment 16 Doug Copi 2006-04-12 08:07:15 PDT
Created attachment 218175 [details]
Demonstration of this bug using frame elements.

The top "google" frame will be moved below the "yahoo" frame using appendChild 10 seconds after the page is loaded.  Any text entered into the google search box will be lost during the move.

tested with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Comment 17 Jesse Ruderman 2006-07-01 01:03:39 PDT
*** Bug 343235 has been marked as a duplicate of this bug. ***
Comment 18 Andreas Grillenberger 2006-10-23 22:12:24 PDT
*** Bug 357700 has been marked as a duplicate of this bug. ***
Comment 19 Michal Till 2007-08-27 06:22:51 PDT
I can confirm that this happens ON 3.0a8pre on Windows.

An iframe reload is triggered even on the simplest DOM operations such as

<div styleid="div1">
    <iframe id="iframe" src="http://www.bbc.com"> </iframe>
</div>
<div id="div2"> </div>

...

document.getElementById("div2").appendChild(document.getElementById("iframe"));
Comment 20 Aiko 2007-10-18 09:37:07 PDT
*** Bug 400284 has been marked as a duplicate of this bug. ***
Comment 21 Mike Moening 2008-01-14 18:28:17 PST
What is the timeframe on a fix for this?
We don't need any more 7 year old bugs out there...
Comment 22 Tom Butler 2008-02-22 04:07:50 PST
With designmode on the same problem occurs which means the contents are lost. This makes moving around a iframe used as a text box for user input very difficult.
Comment 23 Tom Butler 2008-02-22 07:17:51 PST
Sorry, cant edit my reply.

Also, the reference to the frame in window.frames is inexplicably removed.


var iframe = document.getElementsByTagName('iframe')[0];
alert(window.frames[iframe.name]); //Correct. [Object Window] reference.
document.body.appendChild(iframe);
alert(iframe.name); //Correct
alert(window.frames[iframe.name]); //null
Comment 24 Jesse Ruderman 2008-02-22 11:36:52 PST
The window.frames issue is a separate bug (perhaps bug 170799).
Comment 25 Helder "Lthere" Magalhães 2008-02-27 04:26:26 PST
These symptoms suggest relation with bug 302616 and suggest general DOM manipulation issues interfering with iFrame.

Although related bug seems to be improved in recent Firefox 3 builds, no improvements seem to be achieved regarding these test cases (all three test cases were probed using the two versions stated bellow).

Versions:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201
Firefox/2.0.0.12 FireShot/0.32

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022604
Minefield/3.0b4pre
Comment 26 Mike Moening 2008-02-29 12:22:50 PST
Created attachment 306569 [details] [diff] [review]
Patch proposal V1

Here is the start of a patch to fix this bug.  This is not meant to be a final solution to the problem.  However, it does fix both test cases for whatever that's worth!

jst & bz could you gentlemen please enlighten me on what nsGenericHTMLFrameElement::UnbindFromTree() needs to do instead of calling mFrameLoader->Destroy()?  I suspect there must be something we need to do besides nothing.
Also BindToTree() only loads the frame if the loader is null which might not work in all cases.

Mike M.
Comment 27 Reed Loden [:reed] (use needinfo?) 2008-02-29 12:24:24 PST
I think you meant jst@mozilla.org, not djst@mozilla.com. :)
You can use ":jst" to match him.
Comment 28 Mike Moening 2008-02-29 12:27:29 PST
Comment on attachment 306569 [details] [diff] [review]
Patch proposal V1

Sorry. Wrong email in reviewer.. trying again.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2008-02-29 12:45:18 PST
As I said in e-mail, at least the following need to happen:

1)  The docshell needs to be removed from the docshell tree
2)  All the places that expect all docshells to be in a docshell tree (esp. the
    security-related ones) need to be fixed.

Most likely you also need:

3)  Fix the leaks this causes through reference count cycles through the docshell
    (which does not participate in cycle collection).
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2008-02-29 12:49:19 PST
Comment on attachment 306569 [details] [diff] [review]
Patch proposal V1

r- based on bz's above comment.
Comment 31 Mike Moening 2008-02-29 12:59:28 PST
Do you have any ideas on how I can validate the integrity of the doc shell tree after I remove/re-add the frame from it?
Preferably programatically...

>Fix the leaks this causes through reference count cycles through the
docshell
Ideas on how to do this?
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2008-02-29 13:10:05 PST
To be honest, if I knew how to do those three things offhand, I'd just write the code.  The hard part of fixing this bug are those three things.
Comment 33 Jeff Walden [:Waldo] (remove +bmo to email) 2008-02-29 18:49:11 PST
(In reply to comment #31)
> >Fix the leaks this causes through reference count cycles through the
> docshell
> Ideas on how to do this?

Two things mostly related to finding said leaks: first, run Mochitests.  You can do this inside $OBJDIR/_tests/testing/mochitest with the following command:

> python runtests.py --autorun --close-when-done --leak-threshold=0

When the tests finish running you'll get an error message if anything leaks.  (You'll need a debug build to do this, but I assume you're already running with a debug build.)  Currently I have us leaking only an nsTimerImpl and an nsThread in a full test run, so stuff beyond that, particularly if it contains docshells or anything they might conceivably use or entrain, could easily be something a patch of yours might cause.

You may find the patch in bug 420154 useful in getting better leak reporting information, at least until it gets in the tree -- hopefully that's soon.  :-)

Second, write Mochitests that explicitly test iframe moving, loading, reloading, etc. in the normal places and also in "weird" cases, like say doing so from window and iframe onload handlers, mutation events, in the middle of synchronous execution of script within that iframe, etc.  Try passing around references to iframes and their content windows across windows and across and page loads, as well as after the window containing each iframe is closed.  We have lots of documentation on exactly how Mochitests should be written[1], and if you fire up Mochitests without --autorun and/or --close-when-done you can load any individual test and view its source to see how it's written.  (You can also load arbitrary web pages or files in the Mochitest browser to check for leaks in non-Mochitest pages.)

Finally, if you have any other questions regarding Mochitests I'm more than willing to do my best to answer them.
Comment 34 Mike Moening 2008-03-03 12:17:35 PST
Thanks for that helpful info Jeff.
That is what I was looking for!

I've got a more complete patch I'm testing now but the option

--leak-threshold=0

doesn't appear to be a valid option.
Ideas?
Comment 35 Mike Moening 2008-03-03 12:36:41 PST
Also if I run without the --leak-threshold switch I get the following error:

--------------------------------------------
failed to get nsJSRuntimeService!
nsStringStats
 => mAllocCount:        1127
 => mReallocCount:         0
 => mFreeCount:         1055  -- LEAKED 72 !!!
 => mShareCount:        1793
 => mAdoptCount:         153
Server pid: 2292
Timed out while waiting for server statup.
-------------------------

Why is it showing leaks and it hasn't even started yet?
FYI I didn't build using a specific $OBJDIR.  Objects are built in-line with sources.  

Not ideal but works better for me right now since re-builds are not picking up file changes unless I manually delete the .obj file for any file I changed.

Last question:
How can I build faster when I only want to re-build 1 or 2 static libraries and re-link firefox?  
  make -f client.mk build
is what I'm doing now.  The debug cycles are unbelievably slow.
 
Comment 36 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-03 13:59:43 PST
Are you sure you're using runtests.py?  The Perl version that I'm currently trying to terminate with extreme prejudice doesn't support the argument.  For a no-objdir build, I *think* you want python $topsrcdir/_tests/testing/mochitest/runtests.py.

If you want to rebuild only parts, it requires some manual trickery; I'm not sure exactly what that trickery is, but generally building (just "make -C content" or similar) in the top-level directory containing the files you've modified (and browser/app if you're on a Mac) can be enough.
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2008-03-03 14:02:44 PST
If you change content, you have to at least build in content/ and layout/build/.  Depending on your build options, you may also need to relink libxul or whatnot...
Comment 38 Mike Moening 2008-03-03 14:59:28 PST
(In reply to comment #36)
> Are you sure you're using runtests.py? 
Yes.  Look inside runtests.py The word "leak" does not appear in any of the options for this script.

> The Perl version that I'm currently
> trying to terminate with extreme prejudice doesn't support the argument.  
What exactly are you trying to say?

> For a no-objdir build, I *think* you want python
> $topsrcdir/_tests/testing/mochitest/runtests.py.
Yes that is what I'm running and in that directory too.
Here's the exact command I'm running in MINGW32:

python runtests.py --autorun --close-when-done --leak-threshold=0

After it spits out the proper usage junk it says:

runtests.py: error: no such option: --leak-threshold

Without the leak option you get the error in comment #35.
This is more concerning to me.  I'm stuck!
Why doesn't static building work with mochitest?
Comment 39 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-03 15:14:33 PST
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/testing/mochitest/runtests.py.in&rev=1.16&mark=159-165#154

Maybe you need to update your tree?

(As for the Perl thing, I thought it might be that you stumbled on instructions saying to use the Perl version -- it *is* the official one, for now, until I can get the Python one to fully replace it.)
Comment 40 Mike Moening 2008-03-03 15:32:48 PST
All files were originally taken from firefox-3.0b3-source.tar.bz2 with a date of 2/26/2008.

My wincvs has this tag next to the file:  FIREFOX_3_0b3_RELEASE
The runtests.py.in file is at version 1.2.  Quite a bit older than your 1.16 in the link above!
I assume the firefox 3 branch is not up to date with regards to tests?
Can you fix this?
Comment 41 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-03 16:02:20 PST
Realistically, you probably need to be hacking on the trunk code, what you get with |cvs co mozilla/client.mk; cd mozilla; make -f client.mk|.  It's entirely possible that's the version of the file that was around for b3; it hasn't been around that long.
Comment 42 Mike Moening 2008-03-03 16:36:52 PST
Ok I'll update CVS from HEAD.  Honestly, why have a Firefox 3 branch if everybody works off the head?
Comment 43 Reed Loden [:reed] (use needinfo?) 2008-03-03 16:43:21 PST
(In reply to comment #42)
> Ok I'll update CVS from HEAD.  Honestly, why have a Firefox 3 branch if
> everybody works off the head?

We don't have a Firefox 3 branch (yet)... Why do you think there's a branch?
Comment 44 Mike Moening 2008-03-03 16:45:19 PST
Isn't that what FIREFOX_3_0b3_RELEASE is? 
That's what CVS said after pulling down the tar ball firefox-3.0b3-source.tar.bz2.
Comment 45 Reed Loden [:reed] (use needinfo?) 2008-03-03 16:51:19 PST
(In reply to comment #44)
> Isn't that what FIREFOX_3_0b3_RELEASE is? 

No, that's a release tag, which was made when Firefox 3.0 Beta 3 was built. HEAD (trunk) is where Firefox 3 development is actively taking place.
Comment 46 Mike Moening 2008-03-03 20:51:28 PST
Thanks for explaining the difference. The wincvs doesn't really distinquish the two.  Either that or I'm to stupid to understand it.

>No, that's a release tag, which was made when Firefox 3.0 Beta 3 was built.
HEAD (trunk) is where Firefox 3 development is actively taking place.
Regardless shouldn't the tar ball I pulled down have everything in it?  I shouldn't need to go to the HEAD to make unit tests work.  Correct?
Comment 47 Reed Loden [:reed] (use needinfo?) 2008-03-03 21:08:48 PST
(In reply to comment #46)
> >No, that's a release tag, which was made when Firefox 3.0 Beta 3 was built.
> >HEAD (trunk) is where Firefox 3 development is actively taking place.
> Regardless shouldn't the tar ball I pulled down have everything in it?  I
> shouldn't need to go to the HEAD to make unit tests work.  Correct?

The tarball has everything in it from when the tarball was made, as it's a one time thing. Many changes to the unit tests have been made since then, so the tarball is very much out of date, and therefore, if you expect to use new features, you'll need to use the HEAD.
Comment 48 Mike Moening 2008-03-03 21:20:27 PST
Sorry I thought I had pulled down a nightly... Guess that wasn't the case!
Sorry for wasting your time fellas. I'm many hours into pulling down the HEAD from cvs already.

None of this explains the "failed to get nsJSRuntimeService" error I received in comment #35 when trying to run tests, however.  I suspect that problem will still exist when I finally get done re-building again.
Comment 49 Mike Moening 2008-03-04 19:55:44 PST
Created attachment 307387 [details]
 simple testcase that manipulates DOM inside iframe V2

simple testcase that manipulates DOM inside iframe + aref
Comment 50 Mike Moening 2008-03-04 20:10:27 PST
Created attachment 307391 [details] [diff] [review]
Patch proposal V2

This version fixes the bug, passes all mochitests without adding leaks of any kind.

I'm looking for input on the // TODO comments left in the patch as well as more suitable names to replace two methods added by me AddToDocShellTree() & RemoveFromDocShellTree().

There is currently 1 known issue.  I'm not sure what the correct behavior should be. It involves hitting refresh on the main browser window.
Open testcase labeled "simple testcase that manipulates DOM inside iframe V2" and click the link.  It should load google into the IFrame. Now refresh the browser. 
Should the IFrame contain Google or the original "foobar" text?

If this is wrong the issue stems around the fact that GetChildSHEntry() for childOffset zero is returning something different after loading google into the IFrame. I don't understand this mechanism well enough to know what is wrong or how to fix it.  I attempted to as you will see in the patch but fell short.
I doubt its hard to fix but I need help.

Please CC others who have expertise in Frames/DocShell for review and testing.
If you are more qualified than me to finish this feel free to take the bug.
Thanks!
Comment 51 Olli Pettay [:smaug] 2008-03-06 16:21:20 PST
Comment on attachment 307391 [details] [diff] [review]
Patch proposal V2

r- based on IRC discussion.
(Needs lots of testing)
Comment 52 Mike Moening 2008-03-07 07:05:57 PST
The refresh issue I mentioned in #50 is a separate bug that exists with and without my patch.  Bug# 421507 has been filed.
Comment 53 Helder "Lthere" Magalhães 2008-03-07 08:56:12 PST
I believe this may be somehow related with a rather old (but still unconfirmed) issue: bug 302616.
Comment 54 Mike Moening 2008-03-07 08:58:43 PST
When you say "this" what do you mean?
Comment 55 Mike Moening 2008-03-07 09:00:21 PST
I read bug 302616.  Theres no moving of DOM nodes going on. I see no relation sorry.
Comment 56 Mike Moening 2008-03-07 13:15:53 PST
Created attachment 308015 [details]
 simple testcase that manipulates DOM inside iframe

Removed V2 for simple test case now that bug #421507 has been filed.
This patch has no affect on that bug.
Comment 57 Mike Moening 2008-03-07 13:25:19 PST
Created attachment 308016 [details] [diff] [review]
Final Patch

Final patch.
Mochitests coming next...
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 13:29:14 PST
I don't see anything in that patch addressing item 2 from comment 29.  Or item 3, for that matter...
Comment 59 Mike Moening 2008-03-07 13:30:47 PST
Created attachment 308017 [details]
mochitest for this bug

Heres the mochitest file.

This belongs under the mozilla/content/html/content/test when checked in.

Test was verified to contain 2 failures without the patch and sucess with my patch.

This test will verify that unload() is NOT called when a child iframe is removed and re-added to the DOM.
At the same time it verifies that scripts running in the child frame continue to run and are not stopped.
This behavior is the same as in IE 6 & 7.
Comment 60 Mike Moening 2008-03-07 13:40:04 PST
> I don't see anything in that patch addressing item 2 from comment 29.  Or item
> 3, for that matter...

I see zero leaks in mochitest with --leak-threshold=0 is that is what you are referring to.

In regards to item #2 from comment #29.

Show me something that is broke and I'll fix it.
Right now I see nothing.
If you know this code that well (and it sounds like you do) produce a test case that breaks the patch and upload it here.  
I'll be happy to fix anything you find ASAP.
Asking me to vaguely "go find something to fix which might be broken" is asking too much. 
Isn't that what mochitesting is for?
This breaks nothing that I know of.
Maybe you know better.
Show me...
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 14:13:07 PST
> I see zero leaks in mochitest

That just means that nothing is testing the scenarios that would leak. For example, have document A with subdocument B.  Set the frame element that contains B as a property on some node in B.  Then remove that element from document A and forget about it.  As far as I can tell, document B will never get garbage collected.

> Show me something that is broke and I'll fix it.

Any usage of mParentDocument, GetParentDocument is likely broken.  Anything that traverses the docshell tree is likely broken. Anything that looks at root treeitems (of same type or not) is likely broken.  To tell whether it's actually broken requires testing of those codepaths (nonexistent right now) and then code analysis.

> produce a test case that breaks the patch

It would be faster to write the whole patch plus tests myself, to be honest.  See below.

> Asking me to vaguely "go find something to fix which might be broken" is
> asking too much. 

<shrug>.  That's the hard part of fixing this bug: finding all the places that depend on the invariant you're changing and fixing them.  You've done the easy part: changing the invariant.

> Isn't that what mochitesting is for?

Yes, if it had 100% coverage.  Its coverage in this area is closer to 1%, I'd estimate.
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 14:14:40 PST
Put another way, passing the existing tests and the minimal tests that directly test for the bug as originally filed is a necessary but not sufficient condition for a patch being correct.  It must also pass tests that attempt to break the changed code in various ways.  In an ideal world, such tests would exist.  But in reality, they don't, and they need to be written.
Comment 63 Mike Moening 2008-03-10 09:38:08 PDT
> That just means that nothing is testing the scenarios that would leak. For
> example, have document A with subdocument B.  Set the frame element that
> contains B as a property on some node in B.  Then remove that element from
> document A and forget about it.  As far as I can tell, document B will never
> get garbage collected.
Do you have suggestions on how to solve this circular reference problem?
I know this is pretty common on the JS side of things in the GC world.

> Any usage of mParentDocument, GetParentDocument is likely broken.  Anything
> that traverses the docshell tree is likely broken. Anything that looks at root
> treeitems (of same type or not) is likely broken.  To tell whether it's
> actually broken requires testing of those codepaths (nonexistent right now) and
> then code analysis.
I see no breakage as of yet with moderate code coverage. Not done yet either...
Any specific example you could provide would help.

I have one minor annoying issue that I'm working on. When RemoveItem is called immediately after setting the src property to the frame (which causes reload) the reload is completed after the item is removed (as expected)
The issue is that the cursor is still an hourglass since the docshell's mParentWidget member is nulled out by the RemoveItem() see stack below:

-------------------------
>docshell.dll!nsDocShell::SetParentWidget(nsIWidget * aParentWidget=0x00000000)  Line 3794	C++
 gklayout.dll!DocumentViewerImpl::Hide()  Line 1999	C++
 docshell.dll!nsDocShell::SetVisibility(int aVisibility=0)  Line 3907	C++
 gklayout.dll!nsSubDocumentFrame::Destroy()  Line 773	C++
 gklayout.dll!nsBlockFrame::DoRemoveFrame(nsIFrame * aDeletedFrame=0x0a79b5a4, int aDestroyFrames=1, int aRemoveOnlyFluidContinuations=0)  Line 5419	C++
 gklayout.dll!nsBlockFrame::RemoveFrame(nsIAtom * aListName=0x00000000, nsIFrame * aOldFrame=0x0a79b5a4)  Line 5023 + 0x10 bytes	C++
 gklayout.dll!nsFrameManager::RemoveFrame(nsIFrame * aParentFrame=0x0b5ed6c4, nsIAtom * aListName=0x00000000, nsIFrame * aOldFrame=0x0a79b5a4)  Line 695	C++
 gklayout.dll!nsCSSFrameConstructor::ContentRemoved(nsIContent * aContainer=0x03e3ba10, nsIContent * aChild=0x0bdad150, int aIndexInContainer=11, int * aDidReconstruct=0x0012d868)  Line 9649 + 0x12 bytes	C++
 gklayout.dll!PresShell::ContentRemoved(nsIDocument * aDocument=0x0c18b218, nsIContent * aContainer=0x03e3ba10, nsIContent * aChild=0x0bdad150, int aIndexInContainer=11)  Line 4742	C++
 gklayout.dll!nsNodeUtils::ContentRemoved(nsINode * aContainer=0x03e3ba10, nsIContent * aChild=0x0bdad150, int aIndexInContainer=11)  Line 167 + 0xba bytes	C++
 gklayout.dll!nsGenericElement::doRemoveChildAt(unsigned int aIndex=11, int aNotify=1, nsIContent * aKid=0x0bdad150, nsIContent * aParent=0x03e3ba10, nsIDocument * aDocument=0x0c18b218, nsAttrAndChildArray & aChildArray={...})  Line 2842 + 0x11 bytes	C++
 gklayout.dll!nsGenericElement::RemoveChildAt(unsigned int aIndex=11, int aNotify=1)  Line 2775 + 0x2a bytes	C++
 gklayout.dll!nsGenericElement::doRemoveChild(nsIDOMNode * aOldChild=0x0bdad174, nsIContent * aParent=0x03e3ba10, nsIDocument * aDocument=0x0c18b218, nsIDOMNode * * aReturn=0x0012db5c)  Line 3425 + 0x13 bytes	C++
 gklayout.dll!nsGenericElement::RemoveChild(nsIDOMNode * aOldChild=0x0bdad174, nsIDOMNode * * aReturn=0x0012db5c)  Line 2991 + 0x1a bytes	C++
 gklayout.dll!nsSVGUseElement::RemoveChild(nsIDOMNode * oldChild=0x0bdad174, nsIDOMNode * * _retval=0x0012db5c)  Line 91 + 0x14 bytes	C++
 xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000011, unsigned int methodIndex=2, unsigned int paramCount=1235788, nsXPTCVariant * params=0x00000000)  Line 102	C++
-----------------

Finally when the document is done loading we hit this code which takes care of restoring the cursor.  Stack to reach this code is below:

   nsCOMPtr<nsIWidget> mainWidget;
   GetMainWidget(getter_AddRefs(mainWidget));
   if (mainWidget) {
       mainWidget->SetCursor(eCursor_standard);
   }

--------------------------------------
>docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x03c90b04, nsIRequest * aRequest=0x0c0e8a9c, unsigned int aStateFlags=786448, unsigned int aStatus=0)  Line 4920	C++
 docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x03c90b04, nsIRequest * aRequest=0x0c0e8a9c, int aStateFlags=786448, unsigned int aStatus=0)  Line 1236	C++
 docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x0c0e8a9c, unsigned int aStatus=0)  Line 870	C++
 docshell.dll!nsDocLoader::DocLoaderIsEmpty()  Line 765	C++
 docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x082161a8, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 682	C++
 necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x082161a8, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 688 + 0x25 bytes	C++
 gklayout.dll!nsDocument::DoUnblockOnload()  Line 5645	C++
 gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1)  Line 5593	C++
 gklayout.dll!nsDocument::DispatchContentLoadedEvents()  Line 2846	C++
 gklayout.dll!nsRunnableMethod<nsDocument>::Run()  Line 262	C++
 xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f888)  Line 511	C++
 xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0094c650, int mayWait=1)  Line 227 + 0x16 bytes	C++
------------------------------

Can you suggest another way of getting a pointer to the nsGenericHTMLElement or its base class so I can turn off the hourglass cursor (and spinning icon)when the load is complete?

I could simply cache another temporary pointer in docShell which is set whenever RemoveFromDocShellTree() is called and nulled out whenever AddToDocShellTree() is called so that I can always access the parent.

This would mean adding a member to DocShell like:

nsCOMPtr<nsIWidget>        mParentWidgetWhenDetached;

Any pictoral documentation regarding the object model would be nice too have too...
Comment 64 Boris Zbarsky [:bz] (still a bit busy) 2008-03-10 09:47:18 PDT
> Do you have suggestions on how to solve this circular reference problem?

Not offhand, sorry.

I just realized that one other thing to double-check is that the widget and view (and viewmanager) are being properly reparented.  In fact, it might be a good idea to leave the DOM but tear down (or freeze?) the presentation.  The former is what happens for display:none, the latter is what happens for bfcache...  Not sure which is better here.
Comment 65 Mike Moening 2008-03-10 10:04:27 PDT
(In reply to comment #64)
> I just realized that one other thing to double-check is that the widget and
> view (and viewmanager) are being properly reparented.  In fact, it might be a
> good idea to leave the DOM but tear down (or freeze?) the presentation.  The
> former is what happens for display:none, the latter is what happens for
> bfcache...  Not sure which is better here.

Which is former and which is latter?
Please explain your thoughts in more detail. Do you mean leaving the docshell in the tree and "freezing" it instead? If yes please suggest appropriate code examples to illustrate.

Comment 66 Boris Zbarsky [:bz] (still a bit busy) 2008-03-10 15:10:00 PDT
> Which is former and which is latter?

Tearing down and freezing respectively.  And I was talking about the presentation (presshell and prescontext), not the docshell.  I'd explain the thoughts in more detail if there were more detail.  I haven't had a chance to think this whole thing through in detail; doing that is part and parcel of fixing this bug, so it's up to the patch writer and perhaps reviewers.

Doing a search on the string "freeze(" at http://mxr.mozilla.org/ should work for code examples.
Comment 67 Olli Pettay [:smaug] 2008-03-12 05:53:23 PDT
Comment on attachment 308016 [details] [diff] [review]
Final Patch

More testing needed, reference cycle should be tested and fixed.
The patch uses 4 and 2 spaces as indentation. Please be consistent and use the same style as in the file.
Comment 68 Mike Moening 2008-03-12 23:08:21 PDT
I tried freezing the presshell. Using SetSticky() and Freeze().
I unstick and Thaw() when re-appending the IFrame back into DOM with AppendChild().

This has created a small problem.
Since docshell already has a presshell I get an assert because ShowDocShell() doesn't expect mInnerView to be NULL.
See below:

--------------------------
nsresult
nsSubDocumentFrame::ShowDocShell()
{
  nsCOMPtr<nsIDocShell> docShell;
  nsresult rv = GetDocShell(getter_AddRefs(docShell));
  NS_ENSURE_SUCCESS(rv, rv);

  nsCOMPtr<nsIPresShell> presShell;
  docShell->GetPresShell(getter_AddRefs(presShell));

  if (presShell) {
    // The docshell is already showing, nothing left to do...
    NS_ASSERTION(mInnerView, "What's going on?");
    return NS_OK;
  }
-----------------

If I comment out the assert and fill in mInnerView by calling
  CreateViewAndWidget(contentType);
later in the code whenever mInnerView is null I get past the crash.
However now my IFrame has nothing in it!
The screen is showing a black IFrame.

I'm at an impass.  Any help would be greatly appreciated.
Why does ShowDocShell() expect both presshell and mInnerView to be NULL?

I like this solution so far because it keeps docshell's mParentWidget alive.
Which allow asyncronous loading to complete normally after the IFrame has been removed from DOM (It behaves exactly like IE does).

Thanks.
PLEASE HELP.
Comment 69 Mike Moening 2008-03-12 23:09:47 PDT
>The screen is showing a black IFrame.
I meant to say blank NOT black.
Comment 70 cliven 2008-06-22 18:50:40 PDT
Dear All,

Is this problem solved (Iframe reloads when moved around the DOM tree) and where and how to update the patch.

Thank & Best Regards
Clive
Comment 71 cinymini 2008-07-16 09:28:27 PDT
Just as a reference, I've ran the testcase on other browsers:

* Safari 3.1/Win: fails (=reloads the iframes, too)
* Opera 9.5/Win: fails
* IE 7/Win: passes

Sad enough.
Comment 72 Zhan 2009-02-03 19:21:19 PST
Is there any workaround for this "Iframe reloads when moved around the DOM tree" bug? It seems has been there for quite a while.
Comment 73 Mike Moening 2009-02-03 21:38:40 PST
I'm putting this one back in the pool in the hope that somebody who has more expertise and time can be the hero.

The mochitest and test cases I added are still solid either way.

Smaug, Boris? can you guys step up and finally fix this?
Comment 74 Boris Zbarsky [:bz] (still a bit busy) 2009-02-04 09:13:36 PST
If someone else takes other things off my plate, sure...  Therein lies the problem.
Comment 75 mark forster 2010-03-18 07:19:46 PDT
interesting.. .. ran some tests and noticed that this isn't even just about moving the ELEMENT in the DOM but also, if the CSS selectors for that element are changed also.

Using jquery with sortbale DIVS and iframes and using firebug to watch the element, if you move the element slightly, the element never actually moves in the DOM or out of its parent element, however its top,left,height,width and zindex are all changed as well as poition :absolute.

hoping this helps in any way towards a solution?
Comment 76 mark forster 2010-03-18 08:41:24 PDT
Ran a few more tests it this doesn't appear to be related to the css selectors doh! Must be when jquery makes a call to the DOM's insertBefore / appendAfter. i retract my comments ;)

I guess the javascript interpreter and / or firebug are not sufficient enough tools to debug such a low level bug ;)

R(In reply to comment #75)
> interesting.. .. ran some tests and noticed that this isn't even just about
> moving the ELEMENT in the DOM but also, if the CSS selectors for that element
> are changed also.
> 
> Using jquery with sortbale DIVS and iframes and using firebug to watch the
> element, if you move the element slightly, the element never actually moves in
> the DOM or out of its parent element, however its top,left,height,width and
> zindex are all changed as well as poition :absolute.
> 
> hoping this helps in any way towards a solution?
Comment 77 C. Scott Ananian 2010-07-15 22:23:54 PDT
There's a glimmer of a work around.

Webkit has/had the same bug, but as discussed in their bugzilla ( https://bugs.webkit.org/show_bug.cgi?id=13574#c10 ) they've added new functionality to suppress the reload when adoptNode() is used to move the iframe.

This isn't in Mozilla yet (see http://groups.google.com/group/mozilla.dev.tech.dom/browse_thread/thread/960efae1f2ca7bfb?pli=1 ) but when this adoptNode() functionality lands, then you can also use adoptNode() to work around the issue of this bug.

(Mozilla bug 284707 and bug 460460 seem related; I couldn't find a bug which explicitly requested implementation of the webkit adoptNode functionality.)
Comment 78 Nicolas Cynober 2010-08-25 07:42:07 PDT
@Scott thx for the links ! The workaround works nice on webkit and IE does not have this issue. Firefox is the only one remaining :(

@Boris: Damn... With more and more iFrames on the web lately (Facebook is moving from FBML to iFrames), it would be nice to fix it sooner than "Long-term".
Comment 79 Tom Taylor 2011-08-14 10:44:48 PDT
Will we see a fix to adoptNode like webkit based browsers so iframes dont re-execute when moved around the dom!?

Would help with preloading third party banners instead of implementing a js solution due to the awful document.write at the end of the page load!

As there is a new release of FF every 6 weeks or sooner, lets hope we can fit this in asap!
Comment 80 Andreas Brinner 2011-08-22 17:47:32 PDT
Would like to see a solution for this, too.

Just as another use case where this bug is interfering:
WYSIWYG editors (like CKEDITOR) are using iframes for the editor content and moving them around deletes the content, as the iframe is reloaded. That makes it impossible to combine a CKEDITOR with the jQuery sortable plugin, to sort multiple instances of the editor.
Comment 81 Cedric Vivier [:cedricv] 2011-08-22 17:52:23 PDT
Same issue with the Orion editor we are now using as part of browser/devtools.
Comment 82 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 19:11:36 PDT
We could try to specifically hack the "move" operation to not unload the iframe... We'd need to introduce an internal concept of "move" to do that.  :(

The other option is to actually go through and fix all the places that depend on the docshell tree for security stuff to go through the ownerDocument tree instead.
Comment 83 Cedric Vivier [:cedricv] 2011-08-22 22:50:58 PDT
(In reply to Boris Zbarsky (:bz) from comment #82)
> We could try to specifically hack the "move" operation to not unload the
> iframe... We'd need to introduce an internal concept of "move" to do that. 
> :(

Doesn't WebKit introduce this concept 'simply' as "an iframe is moved when it is adopted (adoptNode) by the same document" ?
Comment 84 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 22:57:59 PDT
We can't do that without doing the second half of comment 82, since adoptNode is a straight-out removal from the DOM.
Comment 85 Cedric Vivier [:cedricv] 2011-08-22 23:32:09 PDT
(In reply to Boris Zbarsky (:bz) from comment #84)
> We can't do that without doing the second half of comment 82, since
> adoptNode is a straight-out removal from the DOM.

I'm probably overlooking something, so forvige my naive question, but why would it need a "straight-out removal from the DOM" when adoptNode()'s destination document is the same as the iframe's ownerDocument?

The ownerDocument being unchanged by such a move, the pervasive docshell tree checks might not be failing and/or necessary (to re-perform).
Comment 86 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 23:41:39 PDT
> but why would it need a "straight-out removal from the DOM"

Because the spec says so: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-adopt step 2.  Or http://www.w3.org/TR/DOM-Level-3-Core/core.html#Document3-adoptNode third sentence if you prefer your DOM specs old.

The point is that after the adoptNode call the iframe is no longer in the document tree.  If at any point after that the page in it tries to do something, it needs to be able to reach the right docshells as needed.... even if the node never got reinserted after the adoptNode call.
Comment 87 Tom Taylor 2011-11-17 02:33:12 PST
So how do we fix this issue, as it seems to affect many different user cases?

If you could all vote for this bug to be fixed, we might see some movement soon!


As it seems to be quite important to have this issue resolved, would Mozilla consider including this fix within the next security update for ALL versions of Firefox??? 

Reason i ask is some users may not be able to upgrade to the latest version.
Comment 88 Olli Pettay [:smaug] 2011-11-17 02:48:14 PST
(In reply to Tom Taylor from comment #87)
> would Mozilla
> consider including this fix within the next security update for ALL versions
> of Firefox??? 
No. This is not a critical security or stability bug.

And this is quite a huge new feature to implement, so normal Nightly/Aurora/Beta cycles are certainly needed for testing.
Comment 89 Tom Taylor 2011-11-17 09:07:20 PST
(In reply to Olli Pettay [:smaug] from comment #88)
> (In reply to Tom Taylor from comment #87)
> > would Mozilla
> > consider including this fix within the next security update for ALL versions
> > of Firefox??? 
> No. This is not a critical security or stability bug.
> 
> And this is quite a huge new feature to implement, so normal
> Nightly/Aurora/Beta cycles are certainly needed for testing.

That sounds fair! :)

Once the issue has been resolved i will happily help with testing

I wouldn't have thought that much was involved, although it makes sense as to why this change has been delayed for so many years!
Comment 90 Paul Hoepfner-Homme 2012-03-16 12:38:14 PDT
I haven't been able to find any mention of this, so I'm going to mention it here: In addition to moving an iframe around in the DOM tree, an iframe in Firefox will also reload if it or any of its DOM ancenstors' "position" CSS attribute changes from one value to another. So for instance, I have found that if the iframe is contained in a DIV that starts off having position "relative" and via JavaScript has its position change to "fixed" during the life of the page, that iframe will reload and all navigation context in that iframe will be lost.

In my client's distance learning application, for which we have declared to students that Firefox be recommended browser, we commonly have iframes changing between "embedded" (minimized) view and full-screen (maximized) view. This requires switching the "position" attribute of an ancestor DIV of the iframe from "relative" to "fixed". It works perfectly in WebKit-based browsers, but Firefox causes the frame contents to reload. This is a major inconvenience to students who are working through a Flash-based course embedded in the iframe and have focused on a specific chapter, only to find that when they switch to full-screen mode in Firefox their position in the course is lost.

I would hope that a fix to this bug of moving an iframe around in the DOM tree would also fix the bug when changing the "position" attribute.

Any update on the status of this bug would be most appreciated by my development team, my client, their instructors and their student enrolment. :)
Comment 91 Boris Zbarsky [:bz] (still a bit busy) 2012-03-16 12:53:03 PDT
Paul, there is no reload on style changes in anything resembling a recent gecko version (and here Firefox 1 counts as "recent").  There is a reconstruct on CSS boxes, but that should not affect the DOM or session history or other "navigation" data structures.

Plug-ins in such an iframe will get restarted in Firefox versions before Firefox 13; that was bug 90268.  So chances are, the issue you're running into is fixed in current nightly builds.  Worth trying one to make sure.
Comment 92 Paul Hoepfner-Homme 2012-03-16 13:07:20 PDT
Thank you Boris for the update! I didn't realize there was a bug specifically about that (I guess I just didn't know how to search for it). I'll check the nightly builds.
Comment 93 Hallvord R. M. Steen [:hallvors] 2014-10-07 05:30:47 PDT
It's been a couple of years.. it seems all engines have aligned with Gecko's (and Presto's) behaviour in the meantime! (Also, this used to break Orkut but Orkut is now RIP).

(I've tested with new test cases that load either about:blank or a file from the server, modify the document either through appendChild() or document.write() and moves the IFRAME in the DOM either immediately or from a timeout.)
Comment 94 Hallvord R. M. Steen [:hallvors] 2014-10-07 05:32:43 PDT
Created attachment 8500997 [details]
TC 254144.htm (one of the permutations, still asserts the behaviour this bug wanted to implement is correct)
Comment 95 Hallvord R. M. Steen [:hallvors] 2014-10-09 12:08:43 PDT
Boris, do you know if I can safely assume browser have aligned here, and we can close this bug as invalid? I'm a bit surprised by this conclusion, sounds a bit too good to be true :)
Comment 96 Boris Zbarsky [:bz] (still a bit busy) 2014-10-09 21:54:52 PDT
Doesn't it?  I think it's true, though.
Comment 97 Hallvord R. M. Steen [:hallvors] 2014-10-10 03:02:42 PDT
I'd feel safer if we knew the spec was on our side and had tests in the W3C test suite.. Let's see.

https://html.spec.whatwg.org/multipage/embedded-content.html#the-iframe-element says:

When an iframe element is inserted into a document that has a browsing context, the user agent must create a nested browsing context, and then process the iframe attributes for the "first time".

Interesting spec prose. What does it mean to say "first time" in quotes? I think it means IFRAME attributes should be evaluated every time as if it were the first time - which is good because it means this bug is verified invalid :) - but it's somewhat ambiguous, as in it might mean you only process the attributes for the first time and not later..? The quotes are somewhat weird.

Anne, as you're officially recognised "spec brainbox" ((c) Bruce L) - how do you interpret this, and do you think it could/should be clarified? Also, if I want to add tests, where in the web-platform-tests tree would you expect to find them?
Comment 98 Anne (:annevk) 2014-10-10 03:35:21 PDT
If you follow the link for "process the iframe attributes" it does not seem ambiguous to me at all.
Comment 99 Hallvord R. M. Steen [:hallvors] 2014-10-10 03:55:11 PDT
Any opinion on where tests should go?
Comment 100 Anne (:annevk) 2014-10-10 04:04:48 PDT
In html/browsers/windows or html/semantics/embedded-content/the-iframe-element I guess. I would ask jgraham or zcorpan.
Comment 101 James Graham [:jgraham] 2014-10-10 11:40:53 PDT
If you're testing the "process the iframe attributes for the first time" algorithm, which it sounds like you are, html/semantics/embedded-content/the-iframe-element seems like the right place to me.
Comment 102 Hallvord R. M. Steen [:hallvors] 2014-10-10 11:54:09 PDT
Thanks James, there will be some tests to review very soon :)
Comment 103 Hallvord R. M. Steen [:hallvors] 2014-10-10 12:17:50 PDT
https://github.com/w3c/web-platform-tests/pull/1286
Comment 104 Jesse Ruderman 2014-12-30 08:40:12 PST
*** Bug 361853 has been marked as a duplicate of this bug. ***

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