Closed Bug 1026008 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XBL, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 --- wontfix
firefox33 --- fixed

People

(Reporter: neil, Assigned: neil)

References

Details

(Keywords: regression)

Attachments

(2 files)

[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>.
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.
Blocks: 1004098
Keywords: regression
CC'ing dmajor just in case he's staring at weird Win64 test failures dealing with XBL...
Duplicate of this bug: 1024269
Neil do you know where the bug is or does this still need investigation?
Flags: needinfo?(neil)
Yes, I've already written a quick hack but I haven't had time to finish polishing it yet.
Flags: needinfo?(neil)
So, what's my try chooser syntax to test my patch to see whether it fixes bug 1024269?
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.
Attached patch Lame patchSplinter Review
This works but I don't really like it.
Attached patch Proposed patchSplinter Review
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8441957 - Flags: review?(mrbkap)
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.
Attachment #8441957 - Flags: review?(mrbkap) → review+
(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.
Flags: needinfo?(neil)
https://hg.mozilla.org/mozilla-central/rev/f41bd6b296ea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Duplicate of this bug: 1029164
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
Attachment #8441957 - Flags: approval-mozilla-aurora?
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.)
Attachment #8441957 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.