820799 - preferences.xml tries to set an attribute to null

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[[e-mail]]

Recently setAttribute's behaviour was "corrected" to match the spec which says that null should stringify to "null", however preferences.xml contains the following code block:

var lastPane = null;
if (this.lastSelected) {
  lastPane = document.getElementById(this.lastSelected);
  if (!lastPane) {
    this.lastSelected = null;
  }
}

The lastSelected XBL property wraps an attribute and the code clearly is trying to clear out an invalid lastSelected attribute, however with the new behaviour this fails because the attribute gets set to "null" instead.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

<NeilAway> dao: so, do you advocate null-protection on all of our XBL setters?
<dao> NeilAway: this may be wise, given the possible implications for code we don't control (e.g. add-ons). I have no idea how widespread this issue is, though

Comment 3

[[e-mail]]

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

OK, so this code works in this particular case because we know the last selected pane is always a string or null, but in the general case you could pass 0 or false, would you expect that to stringify to "0"/"false" respectively?

By comparison the built-in nsIDOMXULElement properties appear to stringify null to "" but 0/false/undefined to "0"/"false"/"undefined". Of course we could change that ;-)

Comment 4

[Dão Gottwald [:dao]]

Attachment: alternative patch

(In reply to [e-mail] from comment #3)
> OK, so this code works in this particular case because we know the last
> selected pane is always a string or null, but in the general case you could
> pass 0 or false, would you expect that to stringify to "0"/"false"
> respectively?

I'm not sure, as I don't see why I'd pass 0 or false in the first place.

Anyway, here's a patch that leaves the setter alone, since making it inconsistent with similar setters is probably not a great idea.

Comment 5

[[e-mail]]

(In reply to Dão Gottwald from comment #4)
> (In reply to comment #3)
> > OK, so this code works in this particular case because we know the last
> > selected pane is always a string or null, but in the general case you could
> > pass 0 or false, would you expect that to stringify to "0"/"false"
> > respectively?
> 
> I'm not sure, as I don't see why I'd pass 0 or false in the first place.

Here's an example that I wish I'd thought of before: the selectedIndex property takes and returns a string, even though the index is normally thought of as a number. We couldn't simply clone your original patch and use it for selectedIndex because it wouldn't work for 0. And I think it would be better to have less inconsistency, whether it's not changing any of the setters or changing them all in the same way (which I would be happy with as long as it only affected null itself).

Comment 6

[[e-mail]]

Comment on attachment 691946 [details] [diff] [review]
alternative patch

Actually it might even be worth doing both fixes, this specific case for this bug and the general case for all setters in a separate bug.

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Dão Gottwald [:dao]]

(In reply to [e-mail] from comment #6)
> Comment on attachment 691946 [details] [diff] [review]
> alternative patch
> 
> Actually it might even be worth doing both fixes, this specific case for
> this bug and the general case for all setters in a separate bug.

filed bug 821530

Comment 9

[Ed Morley [:emorley]]

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