1229168 - In Fx 42+, we aren't seeing / collecting heartbeat votes.

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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 1

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

MattN,

0.  I am stumped.  I have been unable to actually reproduce the problem.

1.  So, this is pretty damning:  https://gist.github.com/gregglind/9def0dc05fd51594a4a3

No votes collected for ANY version 42+.  In the same period, SOME votes collected for ALL EARLIER VERSIONS.

2.  I think it has to do with the 'support private browsing mode' OR the 'support engagement surveys' in Tour.  I do not see how.

3.  Other things that would cause this... if a particular HB flow somehow EITHER
 a. 'lost track' of it's `flow_id`
 b. couldn't update it's `updated_ts`

4.  Running *by hand* from the dev tools cli seems to make 'correct' flows that update their ts every stage.

5.  Black swan possibilities... 
  a.  42 somehow changed how notification bars work in the context of multiprocess?
  b.  clocks got... weirder?
  c.  browser.windowManager changes or something?

6.  I have looked at other changes to the heartbeat server.  

7.  see:  https://bugzilla.mozilla.org/show_bug.cgi?id=1229366 .  This suggests that the notification urls are being constructed correctly for the 'engagement' url.

Comment 2

[Matthew N. [:MattN]]

[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.

Comment 3

[Matthew N. [:MattN]]

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

Comment 4

[Matthew N. [:MattN]]

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

Comment 5

[Matthew N. [:MattN]]

OK, I know what's going on :(

Comment 6

[Matthew N. [:MattN]]

Attachment: Minimal patch ignoring the root of the problem

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.

Comment 7

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter

Bug 1229168 - UITour: Use the correct window in initForBrowser to handle windows with no tabs. r=Dexter

Comment 8

[Matthew N. [:MattN]]

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.

Comment 9

[Alessio Placitelli [:Dexter]]

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.

Comment 10

[Alessio Placitelli [:Dexter]]

(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 11

[Matthew N. [:MattN]]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d9fd7783ba30

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Matthew N. [:MattN] from comment #11)
> I assume Dexter just forgot to check the Ship It box.

Yes, sorry.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9fd7783ba30

Comment 16

[Carsten Book [:Tomcat]]

(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

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking for 43-44 since this is a regression - so that we notice if it doesn't land or reopens.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d0c7cabdeb8

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/03351f48deff

Comment 21

[Matthew N. [:MattN]]

(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.

Comment 22

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2d7d6033fae

Comment 23

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c383c019d144

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1d0c7cabdeb8
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e2d7d6033fae

Comment 25

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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.

Comment 26

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Code makes sense.  Current nightly verified works.  I SUPER TOTALLY APPROVE.

Please uplift asap to all channels, as possible.

Comment 27

[Matthew N. [:MattN]]

(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+.