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)

Product:
Firefox
Component:
Tours
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 45
Tracking Flags:
Tracking Status
firefox42 --- wontfix
firefox43 + fixed
firefox44 + fixed
firefox45 --- fixed

People

(Reporter: glind, Assigned: MattN)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter
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?
Assignee: nobody → MattN+bmo
Blocks: 1229366
[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)
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
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
OK, I know what's going on :(
Blocks: 1177162
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.
Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter
Attachment #8694553 - Flags: review?(alessio.placitelli)
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 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)
(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!
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?
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)
(In reply to Matthew N. [:MattN] from comment #11)
> I assume Dexter just forgot to check the Ship It box.

Yes, sorry.
https://hg.mozilla.org/mozilla-central/rev/d9fd7783ba30
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(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.
status-firefox42: affected → wontfix
tracking-firefox43: ? → +
tracking-firefox44: --- → +
(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)
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 → ---
Code makes sense.  Current nightly verified works.  I SUPER TOTALLY APPROVE.

Please uplift asap to all channels, as possible.
Flags: needinfo?(glind)
(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.