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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Doron Rosenberg (IBM)]

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 [:bzbarsky]]

Please attach the testcase to the bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: xul document (part of testcase)

Comment 4

[Doron Rosenberg (IBM)]

Attachment: testcase (main html file)

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

I'll look at this tomorrow. 

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: 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 [: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.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

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]]

We need this on 1.8.0.1 as well.

This is a major regression for us.

Comment 11

[Daniel Veditz [:dveditz]]

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

Comment 12

[Doron Rosenberg (IBM)]

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 [:bzbarsky]]

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)]

checked in for ben

Comment 15

[Doron Rosenberg (IBM)]

Comment on attachment 205336 [details] [diff] [review]
patch

requesting 1.8.0.1 per bz

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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)]

+        mDocumentLoaded = true;

not PR_TRUE?

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Doron Rosenberg (IBM)]

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

Checked in.

Comment 21

[Peter van der Woude [:Peter6]]

could this have caused Bug 322701 ?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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]]

Attachment: Patch to fix regression in bug 322701

Comment 24

[Bill Gianopoulos [:WG9s]]

(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)]

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]]

(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]]

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]]

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 [:bzbarsky]]

This depends on bug 322701 getting approved for branch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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]]

Comment on attachment 205336 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 32

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.0.2

Comment 33

[Jay Patel [:jay]]

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.