974406 - Successfully bookmarking Twitter does not change the bookmark star icon's state

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Andrei Vaida [:avaida]]

User agents:
[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
[3] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
[4] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0

Steps to reproduce:
1. Launch Firefox with a clean profile.
2. Access the following URL: https://twitter.com/
3. Click on the bookmark star icon in order to bookmark the website.

Expected result: The website is added to bookmarks, the bookmark star icon turns yellow.

Actual result: The website is added to bookmarks, the bookmark star icon remains grey.

Additional notes:
1. I managed to reproduce the issue on Windows 7 64-bit, Windows 8.1 32-bit, Ubuntu 13.10 64-bit, Mac OS X 10.8.5, Mac OS X 10.7.5, using:
- the latest Nightly (Build ID: 20140219030203) [1]
- the latest Aurora (Build ID: 20140219004002) [2] 
- the latest Beta (Build ID: 20140218122424) [3] 
- Firefox 27.0.1 (Build ID: 20140212131424) [4]
2. This issue is a regression, window below:
- last good revision: 2013-10-19
- first bad revision: 2013-10-20
3. I was unable to reproduce the bug on Surface Pro 2 tablet (Windows 8.1 Pro 64-bit), using the latest Beta.

Comment 1

[u279076]

Nominating for tracking since this is a regression in Firefox 27.

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e25e62d174ed&tochange=0d316980f21f

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Definitely not respin worthy for FF27, so not tracking there but let's try to find the regressing bug here since it's fairly recent.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Does this reproduce on any other top sites by any chance?

Comment 4

[u279076]

(In reply to Lukas Blakk [:lsblakk] from comment #3)
> Does this reproduce on any other top sites by any chance?

QA reported this bug so I don't think QAWANTED is needed here -- just asking the question is enough. Andrei please check what other sites are affected.

(In reply to Lukas Blakk [:lsblakk] from comment #2)
> Definitely not respin worthy for FF27, so not tracking there but let's try
> to find the regressing bug here since it's fairly recent.

Could we have an engineer look at the pushlog in comment 1? We don't have builds going back far enough to regress this any further.

Comment 5

[u279076]

Nick Thomas pointed out to me that we have archived tinderbox builds going back to October 1st:
http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/

Andrei, please test these builds to see if you can get a more narrow pushlog.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Gavin - can you help us find some potential regressing bugs in that regression range?

Comment 7

[Andrei Vaida [:avaida]]

(In reply to Lukas Blakk [:lsblakk] from comment #3)
> Does this reproduce on any other top sites by any chance?
I was unable to replicate this issue on any other website from the top 50 global sites listed by Alexa [1]. 

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> Nick Thomas pointed out to me that we have archived tinderbox builds going
> back to October 1st:
> http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/
> tinderbox-builds/
> 
> Andrei, please test these builds to see if you can get a more narrow pushlog.
I was able to reproduce the bug on the tinderbox build [2] from 1st of October as well, details:
- Nightly 27.0a1 (2013-10-01)
** build id: 20131001072048 [3]
** revision: http://hg.mozilla.org/mozilla-central/rev/cc4a3f3f899e

- Nightly 28.0a1 (2013-11-07)
** build id: 20131107041709 [4]
** revision: http://hg.mozilla.org/mozilla-central/rev/21b77163bf9f

- Nightly 29.0a1 (2013-12-10)
** build id: 20131210164343 [5]
** revision: http://hg.mozilla.org/mozilla-central/rev/3ea3d3baa67b

- Nightly 30.0a1 (2014-02-20)
** build id: 20140220035628 [6] 
** revision: https://hg.mozilla.org/mozilla-central/rev/cf9666cc3f36 


[1] http://www.alexa.com/topsites 
[2] http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win64/
[3] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:27.0) Gecko/20100101 Firefox/27.0
[4] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:28.0) Gecko/20100101 Firefox/28.0
[5] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:29.0) Gecko/20100101 Firefox/29.0
[6] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 8

[Marco Bonardo [:mak]]

I tried to reproduce on Win8.1 with lastest Nightly but I couldn't, as well as on a retina Mac with 10.9.
Are all of the required steps expressed in comment 0? I suppose they are good to reproduce in a new profile.
In the regression range I can't see anything shouting as culprit.

Comment 9

[Andrei Vaida [:avaida]]

(In reply to Marco Bonardo [:mak] from comment #8)
> I tried to reproduce on Win8.1 with lastest Nightly but I couldn't, as well
> as on a retina Mac with 10.9.
Hi Marco, I successfully reproduced the issue again with the latest Nightly, on a different machine running Windows 8.1 64-bit.

> Are all of the required steps expressed in comment 0? I suppose they are
> good to reproduce in a new profile.
All the necessary steps to reproduce were mentioned in Comment 0 and yes, they're good to reproduce with a new profile.

Comment 10

[Marco Bonardo [:mak]]

ok, I could reproduce once but not in a new profile, it happened intermittently in my usual profile, after it worked properly many times. It seems really intermittent though when it happens looks like something is out-of-sync since you can click many times on the star and it never works until you switch to another tab. It would be nice to have a more persistent way to reproduce, I could only once in tens of tries.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Marco Bonardo [:mak] from comment #10)
> ok, I could reproduce once but not in a new profile, it happened
> intermittently in my usual profile, after it worked properly many times. It
> seems really intermittent though when it happens looks like something is
> out-of-sync since you can click many times on the star and it never works
> until you switch to another tab. It would be nice to have a more persistent
> way to reproduce, I could only once in tens of tries.

I just reproduced this too, in my very oft-used profile, went to https://twitter.com and clicked on the star over and over and it would just go blue & back to grey over and over - no popup of bookmark options.  Switching to another tab and then back did restore the bookmark star being blue again, and clicking on the star brought up the bookmark options box.  Strange that this only happens on Twitter.  Though it's a bit of an edge case, will keep tracking while Marco investigates further since it's a relatively new regression and it's reproducible.

Comment 12

[Marco Bonardo [:mak]]

it is very likely some sort of race condition in BookmarkingUI, but I can't tell until I can reproduce again at least once and catch it in the debugger.

Comment 13

[Marco Bonardo [:mak]]

that said, the regression range still looks very strange if comment 12 is correct, having a smaller range may help, though I figure it's tricky since it's intermittent...

Comment 14

[Andrei Vaida [:avaida]]

I managed to obtain a more narrow regression range using inbound builds:
* Last good revision: 4e7d1e2c93a6
* First bad revision: 07db468de86e
* Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4e7d1e2c93a6&tochange=07db468de86e 

Please let me know if any further bisecting is required.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Andrei Vaida, QA [:AndreiVaida] from comment #14)
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=4e7d1e2c93a6&tochange=07db468de86e 

Unfortunately the above pushlog only contains a patch that cannot in any way influence the star behavior... I think these ranges are very broken due to the intermittent nature of the bug.

Comment 16

[Marco Bonardo [:mak]]

if anyone has more reliable steps to reproduce, please let me know.

Comment 17

[Marco Bonardo [:mak]]

I was able to catch it in a debugger, the problem is here: 

  onItemAdded: function BUI_onItemAdded(aItemId, aParentId, aIndex, aItemType,
                                        aURI) {
    if (aURI && aURI.equals(this._uri)) {

basically, both aURI and this._uri point to https://twitter.com/ (same spec), though the ports differ, looks like there is some internal redirect between 80 and 443, so when we get onLocationChange we set this._uri = gBrowser.currentURI; onLocationChange with port 80, but later when we try to bookmark currentURI its port is 443. When we compare the two through .equals we get false due to the different port.
The fix here is simple, since for the scope of this code we only care about the spec, we can just compare specs.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #17)
> basically, both aURI and this._uri point to https://twitter.com/ (same
> spec), though the ports differ, looks like there is some internal redirect
> between 80 and 443, so when we get onLocationChange we set this._uri =
> gBrowser.currentURI; onLocationChange with port 80, but later when we try to
> bookmark currentURI its port is 443. When we compare the two through .equals
> we get false due to the different port.

That sounds pretty weird. I'm worried there's some deeper issue going wrong here. Can we at least file a bug to followup on the root cause if we just wallpaper over it here?

Comment 19

[Marco Bonardo [:mak]]

I'm not wallpapering it since I found the actual cause, so I'm not going to do the fix in comment 17.
I already have the patch, I was trying to write a test for it but for whatever reason the test doesn't fail even if I reproduce the response from the server.

Comment 20

[Marco Bonardo [:mak]]

Just a brief explanation of what happens:
when we visit the secure version of the page it gives us a Strict-Security-Transport answer, so we add it the to HSTS list. When later the user requests the unsecure version the HSTS list causes us to silently load the secure page. The uri port changes to 443. In transactions code we have a stupid clone doing NetUtil.newURI(oldURI.spec) that loses the port information, so we actually compare an uri on port 443 with the bogus clone.
Now that I added HSTS to the test it is properly failing/succeeding as expected, so patch incoming.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1

Redistributing some reviews load :)

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Oh, nice. So in theory this could affect all sites that use HSTS?

Comment 24

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #23)
> Oh, nice. So in theory this could affect all sites that use HSTS?

yes

Comment 25

[Marco Bonardo [:mak]]

I should recall to never edit the commit message 1 second before pushing :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6487c3f33df9

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6487c3f33df9

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 8383544 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 728230
User impact if declined: Star UI on sites using Strict-Transport-Security doesn't work correct, the star doesn't represent the bookmarking status and breaks until a tab change
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): simple change with a unit test
String or IDL/UUID changes made by this patch: none

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25fd2218dc33
https://hg.mozilla.org/releases/mozilla-beta/rev/8fa484975bc2

Comment 29

[Marco Bonardo [:mak]]

if you have problems reproducing the bug constantly on twitter for verification, you can use the example links in bug 888567, more specifically github.com seems to be quite consistent in behavior.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8fa484975bc2

Comment 31

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on Windows 7 64-bit [1], Windows 8 32-bit [2], Mac OS X 10.9.1 [3] and Ubuntu 13.10 64-bit [4] using Twitter with:
- the latest Beta (Build ID: 20140303165517)
- the latest Aurora (Build ID: 20140304004003)
- the latest Nightly (Build ID: 20140304030204)


[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
[2] Mozilla/5.0 (Windows NT 6.2; rv:28.0) Gecko/20100101 Firefox/28.0
[3] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
[4] Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0