Closed Bug 691951 Opened 13 years ago Closed 13 years ago

Telemetry prompt gets dismissed without user interaction

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
firefox9 --- fixed

People

(Reporter: taras.mozilla, Assigned: Gavin)

References

Details

(Keywords: qawanted, verified-beta, Whiteboard: [qa+], [qa!:9])

Attachments

(2 files, 2 obsolete files)

I noticed I didn't get re-prompted for telemetry today, but the prompted pref has been set correctly.
Keeping this patch until someone comes up with a better suggestion.
Attachment #564698 - Flags: review?
Attachment #564698 - Flags: review? → review?(gavin.sharp)
I experienced this bug because of the way that MAC OSX Lion handles relaunching applications, which is to attempt to open every window of the application that was opened in the previous session.

I updated to 7.0.1, which relaunched Firefox. I got the Telemetry prompt, but a half second later my downloads window launched, which closed the prompt. Probably a problem unique to the way Lion handles application windows, but a problem none the less.
Comment on attachment 564698 [details] [diff] [review]
keep telemetry prompt from disappearing without user interaction

We need to understand why this is happening instead of continuing to wallpaper over this. This is unlikely to help if bug 667592 didn't.
Attachment #564698 - Flags: review?(gavin.sharp) → review-
(In reply to Brian Dils [:briandils] from comment #2)
> I updated to 7.0.1, which relaunched Firefox. I got the Telemetry prompt,
> but a half second later my downloads window launched, which closed the
> prompt. Probably a problem unique to the way Lion handles application
> windows, but a problem none the less.

This seems odd - the download manager shouldn't have any effect on the notification (maybe just a coincidence?).
We need to try and come up with some STR here. Seems like having an existing session to restore is almost certainly involved somehow. Update also being involved makes it tricky I guess.
Keywords: qawanted
Bump on Gavin's request. If you want quick resolution, please provide the steps to reproduce the problem.
(In reply to Dietrich Ayala (:dietrich) from comment #6)
> Bump on Gavin's request. If you want quick resolution, please provide the
> steps to reproduce the problem.

We do not have steps to reproduce the problem. This is why it's hard and requires someone who knows why Firefox behaves in ways it does.
The only theory I have is that restoring a session that includes a selected app tab that is complex and does many redirects might cause the notification to disappear quickly (because we'll reach 6 onLocationChanges very quickly).
Perhaps if we better understood how the prompt is supposed to behave - when it should disappear - we can better discuss situations in which this is a problem today.

As one possible solution (Limi, please weigh in - I know you have strong feelings on this UI) how about simply keeping the prompt up until the user explicitly clicks on something to remove it?
One way to fix this would to be provide a notification when [x] is pressed and another when the infobar is dismissed by other means(whatever they are). Then we could reprompt on next startup and deal with random disappearance this way.
I spoke to zpao about this. The notification we're using to prompt can theoretically fire before the loads have actually been triggered. We can add another notification that fires after the selected tab has actually been restored, and use that instead.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11)
> I spoke to zpao about this. The notification we're using to prompt can
> theoretically fire before the loads have actually been triggered. We can add
> another notification that fires after the selected tab has actually been
> restored, and use that instead.

So this causes it to get dismissed when pageload(s) happens? You are proposing to show that notification bar after session-restore finishes? Out of curiosity, can we make the infobar not get dismissed by pageloads?
(In reply to Taras Glek (:taras) from comment #12)
> So this causes it to get dismissed when pageload(s) happens? You are
> proposing to show that notification bar after session-restore finishes?

Yes, IIRC session store itself causes multiple "loads" just to do the restore, and then the page itself could trigger additional "loads" depending on what it does, I think. I am proposing we show the notification after the first tab's restore has been triggered.

> Out of curiosity, can we make the infobar not get dismissed by pageloads?

Yes - you could set persistence to -1 (a bit of a hack). Another alternative would be to also specify a "timeout" (actually a minimum amount of time that the notification must be shown before being hidden automatically). These are workarounds that would work for telemetry, but we probably want to solve the general problem for the other notifications too.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> (In reply to Taras Glek (:taras) from comment #12)
> > So this causes it to get dismissed when pageload(s) happens? You are
> > proposing to show that notification bar after session-restore finishes?
> 
> Yes, IIRC session store itself causes multiple "loads" just to do the
> restore, and then the page itself could trigger additional "loads" depending
> on what it does, I think. I am proposing we show the notification after the
> first tab's restore has been triggered.

I'm worried that these phantom loads will continue making the prompt disappear for some users(ie a page with lots of ajax).

> 
> > Out of curiosity, can we make the infobar not get dismissed by pageloads?
> 
> Yes - you could set persistence to -1 (a bit of a hack). Another alternative
> would be to also specify a "timeout" (actually a minimum amount of time that
> the notification must be shown before being hidden automatically). These are
> workarounds that would work for telemetry, but we probably want to solve the
> general problem for the other notifications too.

That's what attached patch did, but you said that from further inspection it looked like -1 wouldn't help.
The sessionstore code is very difficult to follow. I'm not confident that I ensured that sessionstore-first-tab-load-started will always fire (particularly in the case of there not being any tabs/sessions to restore). This illustrates the general idea though.
Attachment #570890 - Flags: feedback?(paul)
This is the simpler solution. It's still predicated on my theory being correct - if the notifications are disappearing for some other reason, this might not help.
Comment 3 still applies, though - do we know that bug 667592 didn't help? Do we even have a list of anecdotal reports of this occurring, with build IDs/versions?
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> Comment 3 still applies, though - do we know that bug 667592 didn't help? Do
> we even have a list of anecdotal reports of this occurring, with build
> IDs/versions?

It's been happening ever since telemetry landed. I've only seen it happen once once I landed reprompting and one of my firefox installs didn't reprompt.

Can we also have an event handler fire when the user dismisses telemetry via [x] button(instead of getting dismissed by some other means)? Currently looks like appendNotification accepts a callback after the buttons parameter, but it never gets called in practice. Then we could modify our logic to only set telemetry.prompted pref to true & telemetry to disabled iff we know the user pressed No/[x].
Comment on attachment 570892 [details] [diff] [review]
make all startup notification bars persist indefinitely

Is there a reason there is no r? on either patch?
Comment on attachment 570890 [details] [diff] [review]
delay notifications until the first tab restore has been triggered

Review of attachment 570890 [details] [diff] [review]:
-----------------------------------------------------------------

I know we're looking for something easy, but this feels error prone.

It also doesn't solve the problem of people not restoring their sessions. In fact, I don't think they'll ever see startup notifications so long as they never restore, which is sort of a step sideways.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2759,5 @@
>      if (aTabs.length == 0) {
>        // this is normally done in restoreHistory() but as we're returning early
>        // here we need to take care of it.
>        this._setWindowStateReady(aWindow);
> +      Services.obs.notifyObservers(null, "sessionstore-first-tab-load-started", "");

Do we only want this to go out once per session, similar to "sessionstore-windows-restored"? If so, then we can fire this multiple times (in the multiple window case).

@@ +3122,5 @@
>        if (!didStartLoad)
>          this.restoreNextTab();
> +      else if (!this._firstTabLoadStarted) {
> +        this._firstTabLoadStarted = true;
> +        Services.obs.notifyObservers(null, "sessionstore-first-tab-load-started", "");

Is this necessarily going to fix it? All we've indicated at this point is that a load has started. It should help since we'll skip the about:blank loads, but we could still get into the situation where the page has redirects/whatnot resulting in multiple load events.

And I'm not sure, but if state is something like a single tab which doesn't trigger a load (maybe possible, not sure though) then we wouldn't send the notification out.
Attachment #570890 - Flags: feedback?(paul) → feedback-
So this is probably a more convoluted idea, but why not do the split like you had in attachment #570809 [details] [diff] [review], but have the same general behavior like we do now (no new notifications). Instead we take selectedTab & listen for load events. After a time period (500ms? 1s?) without a load event, we show the notification. Each load event prior to showing the notification would reset the timer. That's still not bulletproof, but takes the dependency out of the twisted paths in sessionstore. We don't have to bump persistence and it handles the other startup paths as well.

Otherwise, just setting persistence = -1 seems like the best bet to me.
Blocks: 697860
(In reply to Paul O'Shannessy [:zpao] from comment #20)
> fact, I don't think they'll ever see startup notifications so long as they
> never restore, which is sort of a step sideways.

The idea was that the notification would always fire, regardless of whether an actual "restore" took place (same as with "sessionstore-windows-restored").

> Do we only want this to go out once per session, similar to
> "sessionstore-windows-restored"? If so, then we can fire this multiple times
> (in the multiple window case).

Good catch, that's wrong.

> loads, but we could still get into the situation where the page has
> redirects/whatnot resulting in multiple load events.

I was operating on the assumption that the main issue is the onLocationChanges triggered by sessionstore itself.

I don't think I will pursue this, though - let's just go with the persistence patch.
Attachment #570892 - Attachment description: make all startup notifications persist indefinitely → make all startup notification bars persist indefinitely
Attachment #570892 - Flags: ui-review?(limi)
Attachment #570892 - Flags: review?(paul)
Attachment #570892 - Flags: review?(paul) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/24129599cb51
Assignee: nobody → gavin.sharp
Flags: in-testsuite-
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → mozilla10
Comment on attachment 570892 [details] [diff] [review]
make all startup notification bars persist indefinitely

Would be nice to backport this to aurora so we don't have to disable telemetry for firefox 9
Attachment #570892 - Flags: approval-mozilla-aurora?
Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4de3faac05

because of a failure in test_resizer.xul on Windows opt builds:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7222752&tree=Mozilla-Inbound
Target Milestone: mozilla10 → ---
We forgot to update the test harness pref when bug 688223 landed, which meant that the notification was showing up in test runs, and this patch made its how up indefinitely rather than just for a few page loads at the beginning.
Attachment #564698 - Attachment is obsolete: true
Attachment #570890 - Attachment is obsolete: true
[triage comment]

This hasn't landed on mozilla-central yet, so we can't take it in aurora yet. Are all the test issues fixed on inbound? Is this planning on landing on central today?
https://hg.mozilla.org/mozilla-central/rev/3b9e99665ee7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #570892 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Just ran into this issue and found an add-on seemed to be responsible.  Filed bug 701857 with a screencast of reproducing.
Whiteboard: [qa+]
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111206 Firefox/11.0a1

Used the following steps (inspired from comment 8):
1. Start Firefox with a clean profile.
2. Open a few tabs, pin an app tab and kill the process.
3. Start Firefox again. (session restore now available)
4. Restart Firefox.
5. Check that the telemetry notification is displayed when restarting after Session restore. 

Are these steps enough to verify this? Or should anything else be added here?
Virgil, the only condition of comment 8 is that the pinned page uses a lot of redirects. Can you confirm this?
Yes, the notification remains displayed even when using a page with lots of redirects as a pinned tab (created a page locally with <meta http-equiv="refresh" content="3;url=..."/>)

Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0

Setting this to verified on Firefox 9 Beta 6 on Ubuntu 11.10, Mac OS 10.6 and Windows 7. 
The notification remains displayed after following the steps in comment 33.
Keywords: verified-beta
Whiteboard: [qa+] → [qa+], [qa!:9]
Attachment #570892 - Flags: ui-review?(limi)
You need to log in before you can comment on or make changes to this bug.