Closed
Bug 974406
Opened 9 years ago
Closed 9 years ago
Successfully bookmarking Twitter does not change the bookmark star icon's state
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
People
(Reporter: avaida, Assigned: mak)
References
Details
(Keywords: regression)
Attachments
(1 file)
7.77 KB,
patch
|
Gavin
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Nominating for tracking since this is a regression in Firefox 27. Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e25e62d174ed&tochange=0d316980f21f
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
tracking-firefox27:
--- → ?
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
Keywords: regression
Comment 2•9 years ago
|
||
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.
(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
Comment 6•9 years ago
|
||
Gavin - can you help us find some potential regressing bugs in that regression range?
Flags: needinfo?(gavin.sharp)
Reporter | ||
Comment 7•9 years ago
|
||
(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
Assignee | ||
Comment 8•9 years ago
|
||
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.
Reporter | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: needinfo?(gavin.sharp)
Comment 11•9 years ago
|
||
(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.
Updated•9 years ago
|
Assignee: nobody → mak77
Assignee | ||
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
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...
Reporter | ||
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
(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.
Assignee | ||
Comment 16•9 years ago
|
||
if anyone has more reliable steps to reproduce, please let me know.
Keywords: steps-wanted
Assignee | ||
Comment 17•9 years ago
|
||
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
Comment 18•9 years ago
|
||
(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?
Assignee | ||
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
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.
Assignee | ||
Comment 21•9 years ago
|
||
Redistributing some reviews load :)
Attachment #8383544 -
Flags: review?(adw)
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: p=0
Comment 23•9 years ago
|
||
Oh, nice. So in theory this could affect all sites that use HSTS?
Updated•9 years ago
|
Attachment #8383544 -
Flags: review?(adw) → review+
Assignee | ||
Comment 24•9 years ago
|
||
(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
Assignee | ||
Comment 25•9 years ago
|
||
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
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6487c3f33df9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8383544 -
Flags: approval-mozilla-beta?
Attachment #8383544 -
Flags: approval-mozilla-beta+
Attachment #8383544 -
Flags: approval-mozilla-aurora?
Attachment #8383544 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Updated•9 years ago
|
Comment 28•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/25fd2218dc33 https://hg.mozilla.org/releases/mozilla-beta/rev/8fa484975bc2
Assignee | ||
Comment 29•9 years ago
|
||
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.
Updated•9 years ago
|
status-b2g-v1.3:
--- → fixed
Reporter | ||
Comment 31•9 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
Updated•9 years ago
|
No longer blocks: fxdesktopbacklog
Whiteboard: p=0
You need to log in
before you can comment on or make changes to this bug.
Description
•