This is probably the wrong component. A display:none'ediframe pointing to a xul document stops main document from finishing to load. Changing to visibility:hidden or a non-xul document works. This used to work in 1.7 and no longer does in 1.8. Testcase linked to in the URL.

Please attach the testcase to the bug.

Not a DOM events bug. This is a regression from bug 282103 -- Ben moved all the code in XULDocument that actually handles end of load into a block that only runs when there is a presentation. That's just majorly wrong. Of course so's the comment that was there before his change...

Attaching the testcase. This html file loads the previously attached xul file.

Note that this happens even if the XUL file just contains <window/>

I'll look at this tomorrow.

Don't use whether or not a reflow has occurred as a hint as to whether or not the document has been loaded once before, instead just use a state flag.

Comment on attachment 205336 [details] [diff] [review] patch Looks good, though no need to init in the constructor -- documents are pre-inited to 0.

Will do, thanks for the fast review Boris. Test case passes. I'll try and get this on the 1.8.1 when it is made (I assume that is very shortly).

We need this on 1.8.0.1 as well. This is a major regression for us.

Has this landed on the trunk? We need a verified, baked patch and this appears to be languishing.

Since this wasn't checked in, should I or is it too late for 1.8.0.1 (since backing time will be only a few days)?

Ben's not gonna be back till far too late. Land this, please. And request 1.8.0.1 approvals. I hate how we don't start triage till a few days before so things will just slip through the cracks. :(

checked in for ben

Comment on attachment 205336 [details] [diff] [review] patch requesting 1.8.0.1 per bz

Comment on attachment 205336 [details] [diff] [review] patch My two cents is that this should be pretty fast and is apparently badly needed by some major customers...

Comment on attachment 205336 [details] [diff] [review] patch I meant "safe", not "fast".... ;)

+ mDocumentLoaded = true; not PR_TRUE?

Er, that should be PR_TRUE. doron, want to fix?

(In reply to comment #19) > Er, that should be PR_TRUE. doron, want to fix? > Checked in.

could this have caused Bug 322701 ?

Comment on attachment 205336 [details] [diff] [review] patch Note that this probably did cause bug 322701. Until we get that sorted out I guess we can't land this on branch. :( It's too bad that we're breaking other XUL consumers for dynamic overlays to work. :(

(In reply to comment #23) > Created an attachment (id=207948) [edit] > Patch to fix regression in bug 322701 > I tested with this patch and the testcase in this bug still appears to work and it fixes the issue with the options panel noted in bug 322701. So far I have not found any other regressions.

Comment on attachment 207948 [details] [diff] [review] Patch to fix regression in bug 322701 > // If this is a dynamic overlay and we have the prototype in the chrome > // cache already, we must manually call ResumeWalk. >- if (aIsDynamic) >+ if (aIsDynamic) { >+ mDocumentLoaded = PR_TRUE; > return ResumeWalk(); >+ } The only other place mDocumentLoaded is set to PR_TRUE is in the if (!mDocumentLoaded) section in ::ResumeWalk. mDocumentLoaded is an indication that the master prototype is done loading. Your patch is setting the done-ness of the master prototype's load state independently of ::ResumeWalk, which seems shaky. Without understanding all possible call patterns, it seems like this could result in race conditions or the document not to be finalized when it should. Why, when ::ResumeWalk is called, is mDocumentLoaded not set to PR_TRUE in the regular place? Does the removal of the placeholder request from the document's load group fail? I'm going to look at this one today too.

(In reply to comment #25) > should. Why, when ::ResumeWalk is called, is mDocumentLoaded not set to PR_TRUE > in the regular place? Does the removal of the placeholder request from the > document's load group fail? (I don't think the problem is that mDocumentLoaded is not being set to PR_TRUE in the regular place. There is a lot of code in ResumeWalk that is only executed if mDocumentLoaded is already set to PR_TRUE when ResumeWalk is called. It appears that in order for the Options panel to display correctly, ResumeWalk needs to be called at least once with mDocumentLoaded already set to PR_TRUE. That does not seem to happen in this case.

OK I think I found what is going on here but have no idea how to fix it. There is a whole section of code in ResumeWlak that can never be reached. This is the sections headed by the following comment: // If we have not yet displayed the document for the first time // (i.e. we came in here as the result of a dynamic overlay load // which was spawned by a binding-attached event caused by // StartLayout() on the master prototype - we must remember that // this overlay has been merged and tell the listeners after // StartLayout() is completely finished rather than doing so // immediately - otherwise we may be executing code that needs to // access XBL Binding implementations on nodes for which frames // have not yet been constructed because their bindings have not // yet been attached. This can be a race condition because dynamic // overlay loading can take varying amounts of time depending on // whether or not the overlay prototype is in the XUL cache. The // most likely effect of this bug is odd UI initialization due to // methods and properties that do not work. This code can only be reached if mDocumentLoaded is PR_TRUE and mInitialLayoutComplete is not PR_TRUE. But the code currently sets both of these to PR_TRUE at the same place so this condition will never occur except that my patch (which set mDocumentLoaded = PR_TRUE while leaving mInitialLayoutComplete false) forced that condition.

Comment on attachment 205336 [details] [diff] [review] patch please resolve the regression issues and get this baked on trunk before renominating for a 1.8.0.x release (but too late for 1801)

This depends on bug 322701 getting approved for branch.

Fixed on 1.8.1 branch. If people are serious about getting this on 1.8.0 branch, I'd request approval...

Comment on attachment 205336 [details] [diff] [review] patch approved for 1.8.0 branch, a=dveditz for drivers

Fixed for 1.8.0.2

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2, testcase finishes loading as expected.