display:none iframe pointing to a xul document stops main document from finishing to load.

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Ben Goodger (use ben at mozilla dot org for email))

Tracking

(Blocks: 1 bug, {fixed1.8.1, verified1.8.0.2})

Trunk
fixed1.8.1, verified1.8.0.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a1 +
blocking1.8.0.1 -
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rft-dl], URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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...
Assignee: events → nobody
Component: DOM: Events → XP Toolkit/Widgets: XUL
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Keywords: helpwanted
QA Contact: ian → xptoolkit.xul
Hardware: PC → All
Blocks: 282103
(Reporter)

Comment 3

12 years ago
Created attachment 205264 [details]
xul document (part of testcase)
(Reporter)

Comment 4

12 years ago
Created attachment 205265 [details]
testcase (main html file)

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. 
Assignee: nobody → bugs
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.
Attachment #205336 - Flags: review?(bzbarsky)
Comment on attachment 205336 [details] [diff] [review]
patch

Looks good, though no need to init in the constructor -- documents are pre-inited to 0.
Attachment #205336 - Flags: review?(bzbarsky) → review+
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

12 years ago
We need this on 1.8.0.1 as well.

This is a major regression for us.
Flags: blocking1.8.0.1?
Attachment #205336 - Flags: superreview?(bryner)
Attachment #205336 - Flags: superreview?(bryner) → superreview+
Has this landed on the trunk? We need a verified, baked patch and this appears to be languishing.
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
(Reporter)

Comment 12

12 years ago
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.  :(
(Reporter)

Comment 14

12 years ago
checked in for ben
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

12 years ago
Comment on attachment 205336 [details] [diff] [review]
patch

requesting 1.8.0.1 per bz
Attachment #205336 - Flags: approval1.8.0.1?
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...
Attachment #205336 - Flags: approval1.8.1?
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?
(Reporter)

Comment 20

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

Checked in.
could this have caused Bug 322701 ?
Depends on: 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.  :(
Created attachment 207948 [details] [diff] [review]
Patch to fix regression in bug 322701
Attachment #207948 - Flags: review?
Attachment #207948 - Flags: superreview?(bryner)
Attachment #207948 - Flags: review?(bzbarsky)
Attachment #207948 - Flags: review?
(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.
Attachment #207948 - Flags: review?(bzbarsky) → review?(bugs)
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.
Attachment #207948 - Flags: review?(bugs) → review-
(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)
Attachment #205336 - Flags: approval1.8.0.1? → approval1.8.0.1-
Attachment #205336 - Flags: approval1.8.1? → branch-1.8.1?(bzbarsky)
Attachment #207948 - Flags: superreview?(bryner) → superreview-
This depends on bug 322701 getting approved for branch.
Attachment #207948 - Attachment is obsolete: true
Attachment #205336 - Flags: branch-1.8.1?(bzbarsky) → branch-1.8.1+
Fixed on 1.8.1 branch.  If people are serious about getting this on 1.8.0 branch, I'd request approval...
Keywords: helpwanted → fixed1.8.1

Updated

12 years ago
Attachment #205336 - Flags: approval1.8.0.2?
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 205336 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #205336 - Flags: approval1.8.0.2? → approval1.8.0.2+
Fixed for 1.8.0.2
Keywords: fixed1.8.0.2

Updated

12 years ago
Whiteboard: [rft-dl]

Comment 33

11 years ago
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.
Keywords: fixed1.8.0.2 → verified1.8.0.2
Flags: blocking1.8.1?

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.