188496 - XBL <constructor> not always called

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[[e-mail]]

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

Comment 1

[[e-mail]]

Attachment: Proposed patch

This appears to resolve the problem by moving AddToAttachedQueue to somewhere
where it is likely to get called.

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 3

[Brendan Eich [:brendan]]

Comment on attachment 111167 [details] [diff] [review]
Proposed patch

Waiting for hyatt to r=.

/be

Comment 4

[[e-mail]]

/me wonders if this relates to any of ere's problems...

Comment 5

[Ere Maijala (slow)]

Sounds like this is practically bug 90337..

Comment 6

[Ere Maijala (slow)]

I'll test if synchronous loading of chrome has any effect on this and something
else.

Comment 7

[Ere Maijala (slow)]

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 8

[[e-mail]]

Comment on attachment 111167 [details] [diff] [review]
Proposed patch

This just makes things worse in the case where the constructors already work
:-(

Comment 9

[[e-mail]]

Attachment: Test case

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 p.foo in a document. The constructor was never called :-(

Comment 11

[Hixie (not reading bugmail)]

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.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

http://glazman.org/weblog/XBL_HTML_test.html

Comment 13

[Hixie (not reading bugmail)]

Seems like the same issue.

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Hixie: nevermind, my mistake. sorry for spam.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: This would fix it

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[[e-mail]]

(In reply to comment #15)
>Created an attachment (id=142105)
>This would fix it
It certainly does.

Comment 18

[Martijn Wargers (dead)]

I can see some similarities with bug 64234 . Maybe that bug would also be fixed
with the patch. 

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: With the better name and location

Agreed on both counts.	Moved the struct to where the other various structs are
declared in nsCSSFrameConstructor.cpp and renamed it to nsAutoEnqueueBinding.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 23

[[e-mail]]

*** Bug 112520 has been marked as a duplicate of this bug. ***

Comment 24

[Martijn Wargers (dead)]

*** Bug 233639 has been marked as a duplicate of this bug. ***