Closed Bug 188496 Opened 22 years ago Closed 20 years ago

XBL <constructor> not always called


(Core :: XBL, defect, P1)






(Reporter: neil, Assigned: bzbarsky)




(2 files, 2 obsolete files)

Steps to reproduce problem:
1. Switch to Classic theme (or other theme that supports text-only)
2. Change your toolbar preference to text-only (or pictures-only)
3. View -> Show/Hide -> Navigation Toolbar (or other primary toolbar)
4. Close and reopen the window
5. Show the primary toolbar hidden in step 3

Expected results: toolbarbuttons are text-only (or pictures-only)

Actual results: toolbarbuttons have pictures and text
Attached patch Proposed patch (obsolete) — Splinter Review
This appears to resolve the problem by moving AddToAttachedQueue to somewhere
where it is likely to get called.
I confirm the problem. I saw that too when I developed my image resizer.
In fact, I guess Neil filed that bug after I told him about the problem.
Comment on attachment 111167 [details] [diff] [review]
Proposed patch

Waiting for hyatt to r=.

Attachment #111167 - Flags: superreview?(brendan)
Attachment #111167 - Flags: review?(hyatt)
/me wonders if this relates to any of ere's problems...
Sounds like this is practically bug 90337..
I'll test if synchronous loading of chrome has any effect on this and something
The problem with pictures/text seems to be that when the navbar is hidden,
LoadBindings() is first called from
nsDOMClassInfo.cpp::nsElementSH::PostCreate(), which doesn't do the same as
nsCSSFrameConstructor. It seems to be somehow timing-related, because I had it
show text and images even when navbar was visible upon startup, when I stepped
through some of the code in debugger. When is the pref change event dispatched?

With the navbar hidden the constructor isn't called. This is the same as bug 90337. 

With syncronous loading, the constructor is always called, although it doesn't
help the text/images issue (I guess because of some timing issue or like).
Comment on attachment 111167 [details] [diff] [review]
Proposed patch

This just makes things worse in the case where the constructors already work
Attachment #111167 - Attachment is obsolete: true
Attachment #111167 - Flags: superreview?(brendan)
Attachment #111167 - Flags: review?(hyatt)
Attached file Test case
Raising radar, this is a must-fix for XBL in my opinion. No embedder can really
rely on XBL if the behavior's constructor is unreliable. Please note that
CrocodileClips people mentioned this bug this week-end at FOSDEM in Brussels!

I can re-confirm this bug : I hit it a few minutes ago trying to attach a super-
simple behavior to all in a document. The constructor was never called :-(
Severity: major → critical
Flags: blocking1.7b?
critical is for "crashes, loss of data, severe memory leak". Reducing to major, 
which is for "major loss of function".

Note that no-one is working on this at the moment.
Severity: critical → major
Hixie, can I ask you to check the test case below, and possibly fork a new
bug if you think it's a new issue ? Thanks a lot.

Seems like the same issue.
Hixie: nevermind, my mistake. sorry for spam.
Flags: blocking1.7b?
Attached patch This would fix it (obsolete) — Splinter Review
Comment on attachment 142105 [details] [diff] [review]
This would fix it

So the issue here is that the newly-created binding needs to be added to "need
to fire constructor" queue.  We weren't doing that in the display:none case. 
Then later, when the classinfo checked whether we needed to, the binding
manager told it "no, someone has already handled it".

This patch makes sure we put the binding in the queue on all returns from
ConstructFrameInternal.  There were two other possible ways to fix this, as far
as I can see:

1)  Just copy/paste those four lines of code into the "display is none" block. 
That seemed unnecessarily error-prone to me for the next time someone adds an
early return codepath.

2)  Add the binding to the queue right after the LoadBindings call (like Neil
tried to, but make sure not to add it twice).  That's sub-optimal because it
will fire the constructor of a binding before the constructors of its anonymous
content, which will likely break things all over.

Hence this approach.
Attachment #142105 - Flags: superreview?(dbaron)
Attachment #142105 - Flags: review?(dbaron)
(In reply to comment #15)
>Created an attachment (id=142105)
>This would fix it
It certainly does.
I can see some similarities with bug 64234 . Maybe that bug would also be fixed
with the patch. 
Comment on attachment 142105 [details] [diff] [review]
This would fix it

I think this new class should have a more specific name --
nsAutoEnqueueBinding, or something like that?  (And perhaps it belongs in the
.cpp file rather than the .h file.)
Attachment #142105 - Flags: superreview?(dbaron)
Attachment #142105 - Flags: superreview+
Attachment #142105 - Flags: review?(dbaron)
Attachment #142105 - Flags: review+
Assignee: hyatt → bzbarsky
OS: Windows 95 → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.7beta
Agreed on both counts.	Moved the struct to where the other various structs are
declared in nsCSSFrameConstructor.cpp and renamed it to nsAutoEnqueueBinding.
Attachment #142105 - Attachment is obsolete: true
Checked in.
Closed: 20 years ago
Resolution: --- → FIXED
*** Bug 112520 has been marked as a duplicate of this bug. ***
*** Bug 233639 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.