Closed
Bug 1229168
Opened 7 years ago
Closed 7 years ago
In Fx 42+, we aren't seeing / collecting heartbeat votes.
Categories
(Firefox :: Tours, defect)
Firefox
Tours
Tracking
()
RESOLVED
FIXED
Firefox 45
People
(Reporter: glind, Assigned: MattN)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
40 bytes,
text/x-review-board-request
|
MattN
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
Observed: 1. In input, no heartbeat "votes" are seen for 42+: version locale votes offers pct | 41.0.1 | en-us | 111 | 2005 | 5.5362 | | 41.0.2 | en-us | 3604 | 50874 | 7.0842 | | 41.0a1 | en-us | 1 | 12 | 8.3333 | | 41.0a2 | en-us | 0 | 17 | 0.0000 | | 42.0 | en-us | 0 | 28936 | 0.0000 | | 42.0a1 | en-us | 1 | 10 | 10.0000 | | 42.0a2 | en-us | 0 | 1368 | 0.0000 | | 43.0 | en-us | 0 | 4134 | 0.0000 | | 43.0a1 | en-us | 0 | 20 | 0.0000 2. Triggering "by hand" seems to work fine. `https://self-repair.mozilla.org/en-US/repair/?{%22runner%22:{%22alwaysRun%22:true}}` Those voted things (which yield the flowid, which I can check), seem to be fine. Asks: 1. Is there any part of this *around 42* that would be affecting any of this?
Comment hidden (obsolete) |
Assignee | ||
Comment 2•7 years ago
|
||
[Tracking Requested - why for this release]: heartbeat data loss. Apparently the team didn't notice until now :( I'm investigating and can reproduce if I set browser.selfsupport.url to: https://self-repair.mozilla.org/%LOCALE%/repair/?{%22runner%22:{%22alwaysRun%22:true}} The JS code is working fine AFAICT and we get to dispatchEvent at https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/content-UITour.js?rev=380817d573cd#99 but the event isn't seen by the page. This is probably related to security wrappers and I'm going to debug with a debug build after lunch.
Status: NEW → ASSIGNED
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
tracking-firefox43:
--- → ?
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 3•7 years ago
|
||
When running the URL from comment 2 in a tab it works fine btw. It doesn't work from the HiddenFrame. I'm seeing this in a debug build which seems like could be related: [56504] WARNING: aTargetFrame should be related with aTargetContent: '!aTargetFrame || !aTargetFrame->GetContent() || aTargetFrame->GetContent() == aTargetContent || aTargetFrame->GetContent()->GetFlattenedTreeParent() == aTargetContent', file /Users/matthew/mozilla-central2/dom/events/EventStateManager.cpp, line 527
Assignee | ||
Comment 4•7 years ago
|
||
Supposed regression range: 7:43.05 LOG: MainThread Bisector INFO Last good revision: 9c919ce631ea (2015-07-19) 7:43.05 LOG: MainThread Bisector INFO First bad revision: 5df788c56ae7 (2015-07-20) 7:43.05 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9c919ce631ea&tochange=5df788c56ae7 13:58.98 LOG: MainThread Bisector INFO Last good revision: f68937aa6842b38eb94b0c338eb00ff9cc15364b 13:58.98 LOG: MainThread Bisector INFO First bad revision: 2d52988a5e8389863f20aa33b04f6d4499563ffe 13:58.98 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f68937aa6842b38eb94b0c338eb00ff9cc15364b&tochange=2d52988a5e8389863f20aa33b04f6d4499563ffe 6:36.17 LOG: MainThread Bisector INFO Last good revision: 081e6aa562f296c2a5157f6633dfe20decc93f92 6:36.17 LOG: MainThread Bisector INFO First bad revision: 879c46e60e5c5052d209e8f4c32d1d7a78047b4a 6:36.17 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=081e6aa562f296c2a5157f6633dfe20decc93f92&tochange=879c46e60e5c5052d209e8f4c32d1d7a78047b4a
Keywords: regression
Assignee | ||
Comment 6•7 years ago
|
||
This is the minimal patch to return to old behaviour. The problem is the hack that we have at https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm?rev=02922ca741fd&mark=368-374#364 which I didn't take into account when I factored initForBrowser to a helper and assumed that I could just do > let window = aBrowser.ownerDocument.defaultView; but in the heartbeat case window changes in the linked hunk. * I'm going to look into why tests didn't catch this and fix that. * I think there is a less fragile approach if I modify notify() to enumerate the hidden window. * There should be monitoring on the server side to detect a lack of data like this.
Assignee | ||
Comment 7•7 years ago
|
||
Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter
Attachment #8694553 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8694480 [details] [diff] [review] Minimal patch ignoring the root of the problem I was looking into having UITour.notify handle the hidden frame/window as I think the current hack for Heartbeat may cause teardown to not work properly (or teardown too early if the window that was the most recent happens to close). I think we can move that to a follow-up, if you agree Dexter, so this lower-risk change to return to old behaviour can be uplifted.
Attachment #8694480 -
Attachment is obsolete: true
Comment 9•7 years ago
|
||
Comment on attachment 8694553 [details] MozReview Request: Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter https://reviewboard.mozilla.org/r/26821/#review24275 This looks good, thanks Matthew.
Attachment #8694553 -
Flags: review?(alessio.placitelli)
Comment 10•7 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #8) > I was looking into having UITour.notify handle the hidden frame/window as I > think the current hack for Heartbeat may cause teardown to not work properly > (or teardown too early if the window that was the most recent happens to > close). > > I think we can move that to a follow-up, if you agree Dexter, so this > lower-risk change to return to old behaviour can be uplifted. Given the importance of having Heartbeat working/this patch uplifted, I agree that investigating the root problem in a follow-up bug may be a good idea!
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8694553 [details] MozReview Request: Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter I assume Dexter just forgot to check the Ship It box. Approval Request Comment [Feature/regressing bug #]: bug 1177162 in Fx42 [User impact if declined]: heartbeat events aren't observable by the heartbeat hidden frame so some data is lost [Describe test coverage new/current, TreeHerder]: More thorough heartbeat test from the Hidden Frame. The majority of the heartbeat tests are testing from a top-level frame in a tab which is why this wasn't caught. [Risks and why]: Very low risk return to old code flow [String/UUID change made/needed]: None
Attachment #8694553 -
Flags: review+
Attachment #8694553 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Attachment #8694553 -
Flags: approval-mozilla-beta?
This may need to wait for the RC build on Monday, but if it lands and we can verify the fix tonight, I could still potentially get it into the beta 9 build tomorrow morning. Tomcat, can this still make tomorrow's nightly? If not then we'll just aim for next Monday. MattN, is this something you could verify for me in nightly, once it's there?
Flags: needinfo?(cbook)
Flags: needinfo?(MattN+bmo)
Comment 14•7 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #11) > I assume Dexter just forgot to check the Ship It box. Yes, sorry.
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9fd7783ba30
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 16•7 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13) > This may need to wait for the RC build on Monday, but if it lands and we can > verify the fix tonight, I could still potentially get it into the beta 9 > build tomorrow morning. > > Tomcat, can this still make tomorrow's nightly? If not then we'll just aim > for next Monday. > > MattN, is this something you could verify for me in nightly, once it's there? this landed on the nightlys
Flags: needinfo?(cbook)
Comment on attachment 8694553 [details] MozReview Request: Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter Fix for recent regression in Heartbeat data.
Attachment #8694553 -
Flags: approval-mozilla-beta?
Attachment #8694553 -
Flags: approval-mozilla-beta+
Attachment #8694553 -
Flags: approval-mozilla-aurora?
Attachment #8694553 -
Flags: approval-mozilla-aurora+
Tracking for 43-44 since this is a regression - so that we notice if it doesn't land or reopens.
Comment 19•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d0c7cabdeb8
Comment 20•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/03351f48deff
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13) > MattN, is this something you could verify for me in nightly, once it's there? No, but Gregg can.
Flags: needinfo?(MattN+bmo) → needinfo?(glind)
Assignee | ||
Comment 22•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e2d7d6033fae
Assignee | ||
Comment 23•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c383c019d144
Comment 24•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1d0c7cabdeb8 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e2d7d6033fae
status-b2g-v2.5:
--- → fixed
Reporter | ||
Comment 25•7 years ago
|
||
From comment 6: "* There should be monitoring on the server side to detect a lack of data like this." Yeah. There should. /me hangs head in shame. Now there is :) It's fixed at the input side, as of three days ago.
status-b2g-v2.5:
fixed → ---
Reporter | ||
Comment 26•7 years ago
|
||
Code makes sense. Current nightly verified works. I SUPER TOTALLY APPROVE. Please uplift asap to all channels, as possible.
Flags: needinfo?(glind)
Assignee | ||
Comment 27•7 years ago
|
||
(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #25) > From comment 6: > > "* There should be monitoring on the server side to detect a lack of data > like this." > Now there is :) It's fixed at the input side, as of three days ago. Thanks. (In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #26) > Please uplift asap to all channels, as possible. Already done. See the comments tagged "uplift" and the tracking flags. This is fixed in Fx43+.
You need to log in
before you can comment on or make changes to this bug.
Description
•