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

VERIFIED FIXED in Firefox 28

Status

()

defect
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: avaida, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27- wontfix, firefox28+ verified, firefox29+ verified, firefox30+ verified, b2g-v1.3 fixed)

Details

Attachments

(1 attachment)

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.
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.
Does this reproduce on any other top sites by any chance?
Keywords: qawanted
(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.
Keywords: qawanted
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.
QA Contact: andrei.vaida
Gavin - can you help us find some potential regressing bugs in that regression range?
Flags: needinfo?(gavin.sharp)
(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
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.
(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.
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.
Flags: needinfo?(gavin.sharp)
(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.
Assignee: nobody → mak77
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.
Status: NEW → ASSIGNED
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...
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.
(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.
if anyone has more reliable steps to reproduce, please let me know.
Keywords: steps-wanted
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.
Keywords: steps-wanted
(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?
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.
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.
Posted patch patch v1Splinter Review
Redistributing some reviews load :)
Attachment #8383544 - Flags: review?(adw)
Blocks: 888567
Duplicate of this bug: 888567
Blocks: 728230
No longer blocks: 888567
Flags: in-testsuite+
Whiteboard: p=0
Oh, nice. So in theory this could affect all sites that use HSTS?
Attachment #8383544 - Flags: review?(adw) → review+
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #23)
> Oh, nice. So in theory this could affect all sites that use HSTS?

yes
I should recall to never edit the commit message 1 second before pushing :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6487c3f33df9
Target Milestone: --- → Firefox 30
https://hg.mozilla.org/mozilla-central/rev/6487c3f33df9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8383544 - Flags: approval-mozilla-beta?
Attachment #8383544 - Flags: approval-mozilla-aurora?
Attachment #8383544 - Flags: approval-mozilla-beta?
Attachment #8383544 - Flags: approval-mozilla-beta+
Attachment #8383544 - Flags: approval-mozilla-aurora?
Attachment #8383544 - Flags: approval-mozilla-aurora+
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.
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
No longer blocks: fxdesktopbacklog
Whiteboard: p=0
You need to log in before you can comment on or make changes to this bug.