As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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)
:
: Neil Deakin
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 User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 2005-12-07 17:03:07 PST
Please attach the testcase to the bug.
Comment 2 User image 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 User image Doron Rosenberg (IBM) 2005-12-07 17:57:40 PST
Created attachment 205264 [details]
xul document (part of testcase)
Comment 4 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Doron Rosenberg (IBM) 2006-01-06 15:25:25 PST
checked in for ben
Comment 15 User image 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 User image 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 User image 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 User image Christian :Biesinger (don't email me, ping me on IRC) 2006-01-06 15:56:49 PST
+        mDocumentLoaded = true;

not PR_TRUE?
Comment 19 User image 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 User image 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 User image Peter van der Woude [:Peter6] 2006-01-07 15:56:18 PST
could this have caused Bug 322701 ?
Comment 22 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 2006-02-14 18:47:42 PST
Fixed for 1.8.0.2
Comment 33 User image 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.