preferences.xml tries to set an attribute to null

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: neil, Assigned: dao)

Tracking

({regression})

Trunk
mozilla20
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 691322 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #691322 - Flags: review?(neil)
(Assignee)

Comment 2

6 years ago
<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
(Reporter)

Comment 3

6 years ago
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 ;-)
(Assignee)

Comment 4

6 years ago
Created attachment 691946 [details] [diff] [review]
alternative patch

(In reply to neil@parkwaycc.co.uk 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.
Attachment #691322 - Attachment is obsolete: true
Attachment #691322 - Flags: review?(neil)
Attachment #691946 - Flags: review?(neil)
(Reporter)

Comment 5

6 years ago
(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).
(Reporter)

Comment 6

6 years ago
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.
Attachment #691946 - Flags: review?(neil) → review+
(Assignee)

Comment 8

6 years ago
(In reply to neil@parkwaycc.co.uk 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
https://hg.mozilla.org/mozilla-central/rev/dc0625023dc0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.