1026008 - XBL child nodes not using document order on Win64 self-build

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[[e-mail]]

[I dogfood with a Win64 self-build. If I use a Win32 self-build on the same profile, I don't observe the issue. I don't know why there would be a difference. I haven't tried a Linux64 build yet.]

SeaMonkey has a custom Preferences window which is loosely based around the Toolkit preferences code but dynamically creates its <prefpane> elements from its preferences tree. Lightning includes an overlay for the window which statically adds its <prefpane> elements. These therefore precede the dynamically created <prefpane> elements.

The <prefpane> elements are then inserted under a <xul:deck> element in SeaMonkey's prefwindow binding using <children includes="prefpane">.

On a 32-bit build this works as expected: the Lightning <prefpane> elements appear first, then the dynamically created <prefpane> elements. This means that you can simply set the <deck>'s selected index to the <prefpane>'s index in the DOM.

However on a 64-bit build Lightning's <prefpane> elements appear after the dynamically created <prefpane> elements! Hilarity ensues. Note that you can use DOM Inspector to "fix" the problem by deleting Lightning's <prefpane> elements from the DOM and then undoing the deletion. This then causes the elements to be re-inserted in the correct order under the <deck>.

Comment 1

[[e-mail]]

Aha! This is a regression from bug 1004098. It changed the type of a variable from uint32_t to size_t, although it's only compared to other uint32_t variables.

Comment 2

[Nathan Froyd [:froydnj]]

CC'ing dmajor just in case he's staring at weird Win64 test failures dealing with XBL...

Comment 4

[Benjamin Smedberg]

Neil do you know where the bug is or does this still need investigation?

Comment 5

[[e-mail]]

Yes, I've already written a quick hack but I haven't had time to finish polishing it yet.

Comment 6

[[e-mail]]

So, what's my try chooser syntax to test my patch to see whether it fixes bug 1024269?

Comment 7

[Timothy Nikkel (:tnikkel)]

You could test locally, just set the pref dom.allow_XUL_XBL_for_file to true and load layout/reftests/dom/multipleinsertionpoints-appendsingle-2.xhtml

trychooser lets you choose win64 http://trychooser.pub.build.mozilla.org/ but I've never tried it.

Comment 8

[[e-mail]]

https://tbpl.mozilla.org/?tree=Try&rev=9a5332a33b15

Comment 9

[[e-mail]]

Attachment: Lame patch

This works but I don't really like it.

Comment 10

[[e-mail]]

Attachment: Proposed patch

I tried making everything consistently size_t but there was too much unrelated 32 bit action going on for me to be sure that I was getting it right.

While I was there I replaced all uses of mInsertedChildren with the relevant public accessors which meant I could make the member private and remove the friends. It also improves the readability of the code.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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

Review of attachment 8441957 [details] [diff] [review]:
-----------------------------------------------------------------

I guess you could have also used a static class member to get the explicit type right? Those stink, though, so r=me.

Comment 12

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ab14487c62

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out this and bug 1025264 in https://hg.mozilla.org/integration/mozilla-inbound/rev/b85caf0e485a for debug crashtest leaks: https://tbpl.mozilla.org/php/getParsedLog.php?id=42093698&tree=Mozilla-Inbound

Comment 14

[[e-mail]]

(In reply to Wes Kocher from comment #13)
> Backed out this and bug 1025264
Actually it was 1026254... so it needed to be backed out for wrong bug# anyway.

Comment 15

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f41bd6b296ea

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f41bd6b296ea

Comment 18

[[e-mail]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1004098
User impact if declined: None, we don't release win64 builds
Testing completed (on m-c, etc.): Fixes tests
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: None

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Aurora- as I don't there is a need to take this change on Aurora. (As stated, we don't ship Win64 builds.)