1112618 - Bookmarks dragged & dropped from special folders onto the bookmarks toolbar display 'null' as a title

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Andrei Vaida [:avaida]]

Reproducible on: Nightly 37.0a1 (2014-12-01) and Aurora 36.0a2 (2014-12-01).
Affected platform(s): Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5.

STR:
1. Launch Firefox with a new profile
2. Enable the Bookmarks Toolbar.
3. Access 'twitter.com' using the Location Bar.
4. From the Most Visited folder, drag & drop the 'twitter.com/' entry onto the Bookmarks Toolbar.
  * note: the entry might be called 'http://twitter.com/' on Mac OS X

ER: The bookmark is properly displayed on the bookmarks toolbar.

AR: The title of the newly dragged bookmark is 'null'.

Screenshot: http://i.imgur.com/22SXc71.png.

Comment 1

[Marco Bonardo [:mak]]

I assume this is without e10s.

I think it's a regression, even if we have no title we should at a maximum display nothing, not "null".

Comment 2

[Alice0775 White]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=87c471673853&tochange=c1425a30de0e

Regressed by: Bug 814195

Comment 3

[Boris Zbarsky [:bzbarsky]]

Presumably some chrome JS calls a Web IDL method declared as DOMString and passes null for the string.  XPCOM used to treat that as "", Web IDL treats it as "null".

Comment 4

[Boris Zbarsky [:bzbarsky]]

And in fact, there is a setAttribute call that passes "null" with this JS stack:

0 PT__insertNewItem(aChild = [xpconnect wrapped nsINavHistoryResultNode @ 0x11b1d03e0 (native @ 0x149ccfe60)], aBefore = null) ["chrome://browser/content/places/browserPlacesViews.js":1019]
    this = [object Object]
1 PT_nodeInserted(aParentPlacesNode = [xpconnect wrapped (nsISupports, nsINavHistoryContainerResultNode, nsINavHistoryQueryResultNode, nsINavHistoryResultNode) @ 0x12bdff6a0 (native @ 0x125e28100)], aPlacesNode = [xpconnect wrapped nsINavHistoryResultNode @ 0x11b1d03e0 (native @ 0x149ccfe60)], aIndex = 2) ["chrome://browser/content/places/browserPlacesViews.js":1215]
    this = [object Object]
2 CITXN_doTransaction() ["resource://gre/modules/PlacesUtils.jsm":2249]
    this = [object Object]
3 ATXN_runBatched(false) ["resource://gre/modules/PlacesUtils.jsm":2133]
    this = [object Object]
4 ATXN_doTransaction() ["resource://gre/modules/PlacesUtils.jsm":2107]
    this = [object Object]
5 anonymous(aTransaction = [object Object]) ["resource://gre/modules/PlacesUtils.jsm":1845]
    this = [xpconnect wrapped native prototype]
6 anonymous() ["chrome://browser/content/places/controller.js":1660]
    this = [object Object]
7 next(val = undefined) ["self-hosted":913]
    this = [object Generator]
8 TaskImpl_run(aSendResolved = true) ["resource://gre/modules/Task.jsm":314]
    this = [object Object]
9 TaskImpl(iterator = [object Generator]) ["resource://gre/modules/Task.jsm":275]
    this = [object Object]
10 anonymous([object Object], [object DataTransfer]) ["resource://gre/modules/Task.jsm":249]
    this = [object Object]
11 PT__onDrop(aEvent = [object DragEvent]) ["chrome://browser/content/places/browserPlacesViews.js":1642]
    this = [object Object]
12 PT_handleEvent(aEvent = [object DragEvent]) ["chrome://browser/content/places/browserPlacesViews.js":1125]
    this = [object Object]

The attribute name is "label".  The top frame there is doing:

1019       button.setAttribute("label", aChild.title);

I don't know offhand whether aChild.title is actually null or "null" here (as in, whether the coercion to string happened before this point).

Comment 5

[Marco Bonardo [:mak]]

makes sense, we should fix that.
For now we are handling both null string or empty string in the title property to distinguish edge case of user setting an empty title VS page without a title, that could probably be simplified since it's not so critical.
Here we could just ensure we don't setAttribute with null.

Comment 6

[Avijit Gupta]

hi there, i would like to work on this bug.

from what i could comprehend:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#1019
before the calling `setAttribute()` , a check needs to be added to ensure that the value of `aChild.title` is not `null`

Comment 7

[Avijit Gupta]

Attachment: Patch - set bookmark label to uri if title is null

added a constraint to check that `aChild.title` is not null , before: http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#1019

[if `aChild.title` is null , then set the `label` to `aChild.uri` instead of `aChild.title`]

Comment 8

[Avijit Gupta]

sir,
i would like to know about the patch that i had proposed. so that it can either be modified or be accepted.

Comment 9

[:Gijs (he/him)]

(In reply to Avijit Gupta from comment #8)
> sir,
> i would like to know about the patch that i had proposed. so that it can
> either be modified or be accepted.

Hi Avijit, sorry for the delay. Can you fix up your patch to use spaces instead of tabs for the indentation, and indent the bits inside the if/else blocks 2 spaces more than the rest?

After that, when you attach it again, please set the review flag to "?" and put "::mak" in the textbox that comes up.

Thank you!

Comment 10

[Marco Bonardo [:mak]]

sorry, please set a reviewer or needinfo flag when you need feedback... we get too much bugmail to notice all the requests as simple comments.

Btw, we should retain the previous behavior here (empty string), not use the uri.
for that I think you could just do something like
aChild.title || ""

Comment 11

[Avijit Gupta]

Attachment: Patch - check that bookmark title should not be `null` before setting it as the label

here is the single patch accomodating all the changes.

sorry for the indentation error btw,
i a really should have noticed it before submitting the patch.

also, i would request that this bug be assigned to me.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8550410 [details] [diff] [review]
Patch - check that bookmark title should not be `null` before setting it as the label

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

Please change the commit message to "Bug 1112618 - Use an empty string instead of null for the label attribute of null-titled bookmarks. r=mak"

Also, there are 2 instances of this bug, the other once is at
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#1869
both should be fixed.

Based on the line numbers I see in your patch, it's likely you need to pull the most recent source code and rebase the patch on top of it.

::: browser/components/places/content/browserPlacesViews.js
@@ +1019,5 @@
> +      if (!aChild.title) {
> +        button.setAttribute("label", "");
> +      } else {
> +        button.setAttribute("label", aChild.title);
> +      }

why not just
button.setAttribute("label", aChild.title || "");

Comment 13

[Avijit Gupta]

Attachment: Patch - check that bookmark title should not be `null` before setting it as the label

made a single patch accommodating all the changesets.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8551757 [details] [diff] [review]
Patch - check that bookmark title should not be `null` before setting it as the label

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

thank you, it looks good.

I'm going to push this to Try

Comment 15

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c87713d43d79

Comment 16

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/6334e1780232

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6334e1780232

Comment 18

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 38.0a1 (2015-01-22) using Ubuntu 12.04 32 bit, Windows 7 64 bit and Mac OS X 10.9.5. 

Per Comment 1, this scenario now results in a bookmark with a blank name instead of 'null'.