Last Comment Bug 319463 - display:none iframe pointing to a xul document stops main document from finishing to load.
: display:none iframe pointing to a xul document stops main document from finis...
Status: RESOLVED FIXED
[rft-dl]
: fixed1.8.1, verified1.8.0.2
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: ---
Assigned To: Ben Goodger (use ben at mozilla dot org for email)
:
:
Mentors:
http://www.nexgenmedia.net/bugs/xulif...
Depends on: 322701
Blocks: 282103
  Show dependency treegraph
 
Reported: 2005-12-07 13:01 PST by Doron Rosenberg (IBM)
Modified: 2008-07-31 03:19 PDT (History)
18 users (show)
bzbarsky: blocking1.9a1+
dveditz: blocking1.8.0.1-
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xul document (part of testcase) (114 bytes, application/vnd.mozilla.xul+xml)
2005-12-07 17:57 PST, Doron Rosenberg (IBM)
no flags Details
testcase (main html file) (224 bytes, text/html)
2005-12-07 17:59 PST, Doron Rosenberg (IBM)
no flags Details
patch (3.88 KB, patch)
2005-12-08 14:04 PST, Ben Goodger (use ben at mozilla dot org for email)
bzbarsky: review+
bryner: superreview+
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.1-
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
Patch to fix regression in bug 322701 (1.23 KB, patch)
2006-01-08 17:40 PST, Bill Gianopoulos [:WG9s]
bugs: review-
bzbarsky: superreview-
Details | Diff | Splinter Review

Description Doron Rosenberg (IBM) 2005-12-07 13:01:17 PST
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2005-12-07 17:03:07 PST
Please attach the testcase to the bug.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2005-12-07 17:20:34 PST
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...
Comment 3 Doron Rosenberg (IBM) 2005-12-07 17:57:40 PST
Created attachment 205264 [details]
xul document (part of testcase)
Comment 4 Doron Rosenberg (IBM) 2005-12-07 17:59:34 PST
Created attachment 205265 [details]
testcase (main html file)

Attaching the testcase.  This html file loads the previously attached xul file.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-12-07 19:04:49 PST
Note that this happens even if the XUL file just contains <window/>
Comment 6 Ben Goodger (use ben at mozilla dot org for email) 2005-12-07 23:07:17 PST
I'll look at this tomorrow. 
Comment 7 Ben Goodger (use ben at mozilla dot org for email) 2005-12-08 14:04:30 PST
Created attachment 205336 [details] [diff] [review]
patch

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 8 Boris Zbarsky [:bz] (still a bit busy) 2005-12-08 14:23:12 PST
Comment on attachment 205336 [details] [diff] [review]
patch

Looks good, though no need to init in the constructor -- documents are pre-inited to 0.
Comment 9 Ben Goodger (use ben at mozilla dot org for email) 2005-12-08 14:44:24 PST
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). 
Comment 10 Mike Kaply [:mkaply] 2005-12-08 15:27:18 PST
We need this on 1.8.0.1 as well.

This is a major regression for us.
Comment 11 Daniel Veditz [:dveditz] 2006-01-06 12:15:25 PST
Has this landed on the trunk? We need a verified, baked patch and this appears to be languishing.
Comment 12 Doron Rosenberg (IBM) 2006-01-06 12:25:43 PST
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)?

Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2006-01-06 15:11:16 PST
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.  :(
Comment 14 Doron Rosenberg (IBM) 2006-01-06 15:25:25 PST
checked in for ben
Comment 15 Doron Rosenberg (IBM) 2006-01-06 15:27:58 PST
Comment on attachment 205336 [details] [diff] [review]
patch

requesting 1.8.0.1 per bz
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2006-01-06 15:31:34 PST
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 17 Boris Zbarsky [:bz] (still a bit busy) 2006-01-06 15:31:57 PST
Comment on attachment 205336 [details] [diff] [review]
patch

I meant "safe", not "fast".... ;)
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2006-01-06 15:56:49 PST
+        mDocumentLoaded = true;

not PR_TRUE?
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2006-01-06 15:58:33 PST
Er, that should be PR_TRUE.  doron, want to fix?
Comment 20 Doron Rosenberg (IBM) 2006-01-06 20:31:30 PST
(In reply to comment #19)
> Er, that should be PR_TRUE.  doron, want to fix?
> 

Checked in.
Comment 21 Peter van der Woude [:Peter6] 2006-01-07 15:56:18 PST
could this have caused Bug 322701 ?
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2006-01-08 17:23:03 PST
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.  :(
Comment 23 Bill Gianopoulos [:WG9s] 2006-01-08 17:40:12 PST
Created attachment 207948 [details] [diff] [review]
Patch to fix regression in bug 322701
Comment 24 Bill Gianopoulos [:WG9s] 2006-01-08 17:43:01 PST
(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 25 Ben Goodger (use ben at mozilla dot org for email) 2006-01-09 11:46:01 PST
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.
Comment 26 Bill Gianopoulos [:WG9s] 2006-01-09 13:48:08 PST
(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.
Comment 27 Bill Gianopoulos [:WG9s] 2006-01-09 18:37:17 PST
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 28 Daniel Veditz [:dveditz] 2006-01-10 14:31:52 PST
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)
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2006-01-30 14:00:31 PST
This depends on bug 322701 getting approved for branch.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2006-02-01 14:00:49 PST
Fixed on 1.8.1 branch.  If people are serious about getting this on 1.8.0 branch, I'd request approval...
Comment 31 Daniel Veditz [:dveditz] 2006-02-14 15:55:23 PST
Comment on attachment 205336 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2006-02-14 18:47:42 PST
Fixed for 1.8.0.2
Comment 33 Jay Patel [:jay] 2006-03-08 16:17:28 PST
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.

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