Permanent orange: TEST-UNEXPECTED-FAIL | multiple-identities/test-display-names.js | test_no_header_name_in_abook_no_pdn (got 'null' but expected 'My Buddy')

RESOLVED FIXED in Thunderbird 20.0

Status

Thunderbird
Address Book
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: aceman)

Tracking

({intermittent-failure, regression})

Trunk
Thunderbird 20.0
intermittent-failure, regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This popped up over the weekend:

SUMMARY-UNEXPECTED-FAIL | test-display-names.js | test-display-names.js::test_no_header_name_in_abook_no_pdn
  EXCEPTION: got 'null' but expected 'My Buddy'
    at: test-display-names.js line 99
       help_test_display_name test-display-names.js 99
       test_no_header_name_in_abook_no_pdn test-display-names.js 159
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183

https://tbpl.mozilla.org/php/getParsedLog.php?id=17541618&tree=Thunderbird-Trunk#error2

Regression range: 

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e6c4d6047db&tochange=ecdf0e332f17

I've currently got a bisect running to investigate further as the failure isn't obvious.
Summary: Permanent orange: TEST-UNEXPECTED-FAIL | multiple-identities/test-display-names.js test_no_header_name_in_abook_no_pdn (got 'null' but expected 'My Buddy') → Permanent orange: TEST-UNEXPECTED-FAIL | multiple-identities/test-display-names.js | test_no_header_name_in_abook_no_pdn (got 'null' but expected 'My Buddy')
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
hg bisect puts the blame on bug 814195.

I suspect there's something that probably needs updating with how we do our header display or something like that.
Blocks: 814195
(Assignee)

Comment 6

5 years ago
Oh, it is in Core therefore we didn't see anything suspicious in c-c commits.

Updated

5 years ago
Keywords: regression
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 12

5 years ago
Mconley, I can see these nulls displayed while using the current trunk build. It happens if the To recipient has the yellow star beside the name in the header pane. The recipient is in an addressbook. For me, the recipient was in the Collected addresses addressbook. As soon as I deleted him from it, the address displayed properly in the msg header pane. I didn't even need to reselect the msg or anything.
(Assignee)

Comment 13

5 years ago
For the mentioned recipient address that gets passed in msgHdrViewOverlay.js::FormatDisplayName(), this function returns null. I'll 
check this out.
(Assignee)

Comment 14

5 years ago
Well, the result of FormatDisplayName function is not null but "null" (a string) now. And that is no longer falsy as we expect in later code.

It is because aDocumentNode.getAttribute("displayName") returns "null" (string). And that is because before that we .setAttribute("displayName", null) [not string]. Bz, is that something that the patch in bug 814195 intended?
(Assignee)

Comment 15

5 years ago
So it looks like we .setAttribute() to null (typeof = "Object"), but from.getAttribute() we get "null" (typeof = "string").
Flags: needinfo?(bzbarsky)
Yes, the behaviour changed to be more spec-compliant. You should set the attribute to the empty string instead of null.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 17

5 years ago
OK, thanks.

I wonder if this does not bite us at many more places.
(Assignee)

Comment 18

5 years ago
Created attachment 690126 [details] [diff] [review]
patch

After the Core patch .getAttribute of an attribute that does not exist returns "". It seems that is usable for us too, instead of null.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #690126 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 19

5 years ago
I used .removeAttribute when the value of the attribute would be null. But we can set it to "" instead. Whatever squib decides.

Comment 20

5 years ago
(In reply to :aceman from comment #14)
> Well, the result of FormatDisplayName function is not null but "null" (a
> string) now. And that is no longer falsy as we expect in later code.

Are you sure about this? If the address isn't your identity and you don't have an AB card for it, it will return null. The only way it can return the empty string is if you have an AB card and either the email address from the mail header has no display name or the card's display name is blank.

(In reply to :aceman from comment #19)
> I used .removeAttribute when the value of the attribute would be null. But
> we can set it to "" instead. Whatever squib decides.

I think this should be equivalent to what you're doing now, since getAttribute on a non-existing attributes returns "" (at least in XUL):

  aEmailNode.setAttribute("fullAddress", aAddress.fullAddress || "");
Just to clarify what changes: passing null to setAttribute used to convert it to "" internally and set the attribute to "".

That got changed in bug 814195 to follow the spec and convert null to "null", which matches other browsers.

GetAttribute(), on the other hand, did not change behavior.  It used to return the string that was set ("") and now returns the string that was set ("null").

Note that the behavior described in comment 20, where in XUL getAttribute of a nonexistent attribute returns "", is technically a bug: per spec getAttribute of a nonexistent attribute should in fact return null.  We do that for all non-XUL elements...

Comment 22

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #21)
> Note that the behavior described in comment 20, where in XUL getAttribute of
> a nonexistent attribute returns "", is technically a bug: per spec
> getAttribute of a nonexistent attribute should in fact return null.  We do
> that for all non-XUL elements...

That's odd. I was just going by what MDN says: https://developer.mozilla.org/en-US/docs/DOM/element.getAttribute#Notes
(Assignee)

Comment 23

5 years ago
(In reply to Jim Porter (:squib) from comment #20)
> (In reply to :aceman from comment #14)
> > Well, the result of FormatDisplayName function is not null but "null" (a
> > string) now. And that is no longer falsy as we expect in later code.
> 
> Are you sure about this? If the address isn't your identity and you don't
> have an AB card for it, it will return null.
According to comment 12, there was a card for the address. But in that case 
FormatDisplayName first used the aHeaderDisplayName argument, which now was "null" string. As "PreferDisplayName" was not set in the card, the returned displayName stayed at the value of the string "null".

> The only way it can return the
> empty string is if you have an AB card and either the email address from the
> mail header has no display name or the card's display name is blank.

> I think this should be equivalent to what you're doing now, since
> getAttribute on a non-existing attributes returns "" (at least in XUL):
> 
>   aEmailNode.setAttribute("fullAddress", aAddress.fullAddress || "");
Yeah, this is shorter, I'll use this.
(Assignee)

Comment 24

5 years ago
Created attachment 690134 [details] [diff] [review]
patch v2
Attachment #690126 - Attachment is obsolete: true
Attachment #690126 - Flags: review?(squibblyflabbetydoo)
Attachment #690134 - Flags: review?(squibblyflabbetydoo)

Comment 25

5 years ago
Comment on attachment 690134 [details] [diff] [review]
patch v2

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

The code looks good. Don't change the comment though; you'll still get back null whenever there's no card for the address, which is what the comment talks about.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1224,5 @@
>  /**
>   * Take an email address and compose a sensible display name based on the
>   * header display name and/or the display name from the address book. If no
>   * appropriate name can be made (e.g. there is no card for this address),
> + * returns "".

Please don't change this comment. The old way is correct.

@@ +1230,5 @@
>   * @param aEmailAddress       the email address to format
>   * @param aHeaderDisplayName  the display name from the header, if any
>   * @param aContext            the field being formatted (e.g. "to", "from")
>   * @param aCard               the address book card, if any
> + * @return                    The formatted display name, or the empty string.

Likewise here.
Attachment #690134 - Flags: review?(squibblyflabbetydoo)

Comment 26

5 years ago
If you prefer, you can adjust the implementation of FormatDisplayName to return null instead of the empty string when there's a card.
(Assignee)

Comment 27

5 years ago
That's what I wanted to say:) It can return null, "", or an address. I'll try to merge the null and "".
(Assignee)

Comment 28

5 years ago
What would you prefer in case aHeaderDisplayName is empty and there is a card but the Display name in it is empty?
(In reply to Boris Zbarsky from comment #21)
> Just to clarify what changes: passing null to setAttribute used to convert
> it to "" internally and set the attribute to "".
> 
> That got changed in bug 814195 to follow the spec and convert null to
> "null", which matches other browsers.
Is there an easy way of tweaking my build to assert whenever chrome code tries to pass null to setAttribute, as this new behaviour is almost certainly useless?
(Assignee)

Comment 30

5 years ago
Neil, can you see if SM needs the same fix? It also sets these attributes unconditionally in updateEmailAddressNode() and then reads it in  UpdateEmailNodeDetails().

Comment 31

5 years ago
(In reply to :aceman from comment #28)
> What would you prefer in case aHeaderDisplayName is empty and there is a
> card but the Display name in it is empty?

I'd do something like this:

1262   if (card) {
1263     if (!displayName && aHeaderDisplayName)
1264       displayName = aHeaderDisplayName;
1265 
1266     // getProperty may return a "1" or "0" string, we want a boolean
1267     if (!displayName || card.getProperty("PreferDisplayName", true) != false)
1268       displayName = card.displayName || null;
1269   }
> I was just going by what MDN says

MDN is talking about the old DOM Core spec, which as it notes does not reflect reality.  Spec's been updated to do so...

> Is there an easy way of tweaking my build to assert whenever chrome code tries to pass
> null to setAttribute

You could check nsContentUtils::IsCallerChrome() && DOMStringIsNull(aValue) in Element::SetAttribute.
I fixed the MDN docs.
(Assignee)

Comment 34

5 years ago
Which ones?
https://developer.mozilla.org/en-US/docs/DOM/element.setAttribute is not clear about it.
(Assignee)

Comment 35

5 years ago
Created attachment 690186 [details] [diff] [review]
patch v3
Attachment #690134 - Attachment is obsolete: true
Attachment #690186 - Flags: review?(squibblyflabbetydoo)
(In reply to Boris Zbarsky from comment #32)
> > Is there an easy way of tweaking my build to assert whenever chrome code tries to pass
> > null to setAttribute
> You could check nsContentUtils::IsCallerChrome() && DOMStringIsNull(aValue)
> in Element::SetAttribute.
Doesn't work, aValue is already "null" by then. Fortunately we don't have many people trying to set an attribute to "null" so I just used EqualsLiteral.

(In reply to aceman from comment #30)
> Neil, can you see if SM needs the same fix? It also sets these attributes
> unconditionally in updateEmailAddressNode() and then reads it in 
> UpdateEmailNodeDetails().
Looks like we do. My assertion also found one other caller - in mailWindow.js myDefaultStatus defaults to null when it should be "" (I think you don't notice because after the start page loads it gets correctly reset to "").

Comment 37

5 years ago
Comment on attachment 690186 [details] [diff] [review]
patch v3

Looks good, assuming all the tests pass.
Attachment #690186 - Flags: review?(squibblyflabbetydoo) → review+
Comment hidden (Treeherder Robot)
(Assignee)

Comment 39

5 years ago
The test in the summary does pass now.

Neil, I could blindly do a patch for SM but without testing. It would probably be better if somebody from SM could do it. Will you create a new bug or do you make the patch here?
Keywords: checkin-needed

Updated

5 years ago
Blocks: 819798
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Many thanks for fixing this aceman!

https://hg.mozilla.org/comm-central/rev/a86f2efe5ffa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in before you can comment on or make changes to this bug.