Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] Bug 1727776, #1599 - Attempt at fixing Glean.js + iOS issues #1854

Closed

Conversation

brizental
Copy link
Contributor

@brizental brizental commented Sep 21, 2021

Related to: #1599

Hey folks!

This is a "do not merge" PR because it contains an unreleased version of Glean.js.

tl;dr; I was able to fix the issue for Release builds, but the issue persists in Debug builds. The current crash is caused by a bug in the Qt JS interpreter and I was not able to figure out a change in the JS code that does not cause it. Since it happens only in Debug mode, I would like to know from you if this is a good enough situation for now. I will file a bug against the Qt bug tracker in order to see if that gets fixed upstream, but that is a slow process for sure. If possible I would like for you to test it out as well to see if you also can't reproduce the bug in Release builds anymore.

There are many changes to Glean.js here -- including extremely verbose logs, those will be turned off in the real release. The commit history on the Glean.js PR has one by one the changes I made and why. On the description of that PR there is also a detailed description of my investigation efforts.

Regarding the VPN app changes, during my investigations I was working with the newest version of Glean.js, so I also updated Glean.js and glean_parser in here for testing. I also re-enabled Glean in all platforms, but that would probably need to be undone in order to disable it in iOS debug mode.

@brizental brizental marked this pull request as draft September 21, 2021 16:03
@brizental
Copy link
Contributor Author

brizental commented Sep 21, 2021

Ack, I messed up on the git history here. Please refrain from testing for the moment, I will fix it as soon as I leave the meeting I am currently on. It's fixed, let me know if it works out.

Note: It is not necessary to check if Glean has upload enabled when
submitting pings as Glean already does that internally.
@brizental brizental requested a review from birdsarah September 21, 2021 16:54
@@ -63,33 +63,11 @@ Window {
minimumWidth = Theme.desktopAppWidth
}

Glean.initialize('MozillaVPN', VPNSettings.gleanEnabled && VPNFeatureList.get("glean").isSupported, {
Glean.setLogPings(true);
Glean.setDebugViewTag("vpn-testing");
Copy link
Contributor Author

@brizental brizental Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tag is a debugging feature. Setting this means all pings will be tagged upon sending, in practice that means the pings are re-routed to Glean's debug ping viewer. You can see the ping I sent when testing this here: https://debug-ping-preview.firebaseapp.com/pings/vpn-testing, subsequent pings sent with this tag will also show up there.

@birdsarah
Copy link
Contributor

EXCELLENT - I'll take a look

@brizental
Copy link
Contributor Author

Quick update here. The crash I described has been confirmed as a Qt JS interpreter bug. See: https://bugreports.qt.io/browse/QTBUG-96788

@birdsarah
Copy link
Contributor

Closing as this PR has served it's purpose and 0.21 of glean is released and we'll be working on integration. thanks so much @brizental .

@birdsarah birdsarah closed this Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants