Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Cleanly reject pings that are submitted too late in shutdown

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: gfritzsche, Assigned: fionn_mac, Mentored)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

Per bug 1301289, submitting pings to Telemetry after the shutdown blocker was resolved results in an error message that is not very clear.

We should reject this and log a clear error, mentioning to not submit pings at the latest per "profile-before-change", "profile-before-change-telemetry" or AsyncShutdown.sendTelemetry.

(Mark Hammond [:markh] from bug 1301289, comment #0)
> I noticed on shutdown an error in the console:
> 
> > 1473309697806 Sync.Telemetry    TRACE   observed xpcom-shutdown null
> > JavaScript error: resource://gre/modules/AsyncShutdown.jsm, line 693: Error: Phase "TelemetryController: Waiting for pending ping activity" is finished, it is too late to register completion condition "Waiting for ping task"
> 
> The problem is that we are attempting to submit a ping on the xpcom-shutdown
> observer - but by this point the process has torn down so far that the ping
> can't be archived. about:telemetry thus never shows these pings.

Comment 1

11 months ago
Hi,

I'd like to work on this. How can I get started?
This can be fixed by adding a "_shuttingDown" boolean flag to TelemetryController at [1]. The flag should be initially set to false, and set to true when Firefox shuts down [2].

With that in place, we're now able to reject pings sent after the shutdown. We can do that by checking if |_shuttingDown| is true and, if that's the case, logging the message mentioned in comment 0 and returning an appropriate message in |Promise.reject()|.

[1] - https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/toolkit/components/telemetry/TelemetryController.jsm#322
[2] - https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/toolkit/components/telemetry/TelemetryController.jsm#784,800
[3] - https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/toolkit/components/telemetry/TelemetryController.jsm#505
In order to make sure this change works, we should add a new test to the |test_TelemetryController.js| file, that shuts down Telemetry using |yield TelemetryController.testShutdown()| and then tries to send a ping.

Take a look at the other tests in the file to see how that can be done.

In order to trigger the test, you can use the following commands:

- ./mach test toolkit/components/telemetry/tests/unit/test_TelemetryController.js
- ./mach test toolkit/components/telemetry

The first one only runs the tests for TelemetryController, while the seconds runs all the Telemetry XPCSHELL tests. Please use both to check that your patch is working and not regressing anything.

Comment 4

10 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #3)
> In order to make sure this change works, we should add a new test to the
> |test_TelemetryController.js| file, that shuts down Telemetry using |yield
> TelemetryController.testShutdown()| and then tries to send a ping.
> 
> Take a look at the other tests in the file to see how that can be done.
> 
> In order to trigger the test, you can use the following commands:
> 
> - ./mach test
> toolkit/components/telemetry/tests/unit/test_TelemetryController.js
> - ./mach test toolkit/components/telemetry
> 
> The first one only runs the tests for TelemetryController, while the seconds
> runs all the Telemetry XPCSHELL tests. Please use both to check that your
> patch is working and not regressing anything.

Thanks Alex. Sorry for the delay. I'll get started on it, and report back with what happens. 

Best Regards,
Wasif Hyder
(Assignee)

Comment 5

7 months ago
Hello,

Is anyone working on this bug currently?
If not, may I give it a go?
(Assignee)

Comment 6

7 months ago
Created attachment 8821776 [details] [diff] [review]
changes.diff

Hello,
Since no one replied to my original comment, I thought I might try solving this issue.
I've attached a diff file which contains the changes I've made, but the changes in TelemetryController.jsm do not pass the new test added by me to test_TelemetryController.js . Instead, "Unexpected exception Error: Ping submitted after shutdown. at resource://gre/modules/TelemetryController.jsm:500" is the result.

I'm still a beginner, and I have no idea why the above mentioned error might occur. However, if someone could point me in the right direction regarding the correctness of changes I've made, the cause of this error and/or the correct way to solve it, then I'd be really obliged :)

Also, Merry Christmas!
Flags: needinfo?(alessio.placitelli)
(Assignee)

Updated

7 months ago
Attachment #8821776 - Attachment is patch: false
Flags: needinfo?(alessio.placitelli)
(Assignee)

Updated

7 months ago
Flags: needinfo?(alessio.placitelli)
Assignee: nobody → vedant.sareen
Status: NEW → ASSIGNED
(In reply to Vedant Sareen from comment #6)
> Created attachment 8821776 [details] [diff] [review]
> changes.diff
> 
> Hello,
> Since no one replied to my original comment, I thought I might try solving
> this issue.

Hi Vedant and thanks for the patch! I've assigned the bug to you, sorry for the delay!

> I've attached a diff file which contains the changes I've made, but the
> changes in TelemetryController.jsm do not pass the new test added by me to
> test_TelemetryController.js . Instead, "Unexpected exception Error: Ping
> submitted after shutdown. at
> resource://gre/modules/TelemetryController.jsm:500" is the result.

That's indeed a great start. Rather than submitting a diff file, would you mind
generating a patch file following the instructions at [1]? Since you're not using
MozReview, it should be as simple as doing "hg bzexport", as long as you have your
repository properly configured.

As for the error, it means that your patch is probably working correctly, so.. congrats! :)
We just need to tweak the tests a little bit (see my next comment).

> Also, Merry Christmas!

Merry Christmas to you too!

[1] - https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Flags: needinfo?(alessio.placitelli) → needinfo?(vedant.sareen)
Attachment #8821776 - Attachment is patch: true
Comment on attachment 8821776 [details] [diff] [review]
changes.diff

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

This looks good overall. Please address the comments below.

Don't forget to generate a patch file with a commit message.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +320,4 @@
>  var Impl = {
>    _initialized: false,
>    _initStarted: false, // Whether we started setting up TelemetryController.
> +  _shuttingDown: false, // Whether the browser is shutting down

nit: add the missing trailing "."

@@ +494,4 @@
>    submitExternalPing: function send(aType, aPayload, aOptions) {
>      this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
>  
> +    //Reject pings sent after shutdown

nit: please add a whitespace between // and Reject, so that it reads "// Reject". Also add the missing punctuation to be consistent with the rest of the comments within the file.

@@ +494,5 @@
>    submitExternalPing: function send(aType, aPayload, aOptions) {
>      this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
>  
> +    //Reject pings sent after shutdown
> +    if (!this._initStarted && this._shuttingDown) {

We don't need to check for |!this._initStarted| here. We only need to check if we're shutting down.

@@ +811,4 @@
>      //   4) _delayedInitTask finished running already.
>  
>      // This handles 1).
> +    if (!this._initStarted && !this._shuttingDown) {

The condition didn't need to change, it was correct.

You just need to add a |this._shuttingDown = true;| before the |return Promise.resolve();|.

This is needed to cover the edge case of shutting down Firefox before Telemetry is fully initialized.
Attachment #8821776 - Flags: feedback+
Comment on attachment 8821776 [details] [diff] [review]
changes.diff

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +495,5 @@
>      this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
>  
> +    //Reject pings sent after shutdown
> +    if (!this._initStarted && this._shuttingDown) {
> +      this._log.error("Ping submitted after shutdown");

Since we're reporting the same message here, we could create a |const| holding the message and pass it both the logger and the |reject|.

Maybe it would be a good idea to also log the |aType|.
(Assignee)

Comment 10

7 months ago
Thank you for the feedback, Sir :)
I apologise for the deviation from the standards in the comment messages. As asked, I've made the changes, and I will definitely keep this in mind from now on.

(In reply to Alessio Placitelli [:Dexter] from comment #9)
> 
> Review of attachment 8821776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryController.jsm
> @@ +495,5 @@
> >      this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
> >  
> > +    //Reject pings sent after shutdown
> > +    if (!this._initStarted && this._shuttingDown) {
> > +      this._log.error("Ping submitted after shutdown");
> 
> Since we're reporting the same message here, we could create a |const|
> holding the message and pass it both the logger and the |reject|.
> 
> Maybe it would be a good idea to also log the |aType|.

I kept the logged message and the reject error the same as I couldn't really think of a suitable message to be logged. I meant to ask this too in my previous comment, it must've slipped my mind.

> We should reject this and log a clear error, mentioning to not submit pings
> at the latest per "profile-before-change", "profile-before-change-telemetry"
> or AsyncShutdown.sendTelemetry.

Since we also need to mention not to submit pings, what exactly should the logged message be?
Or is the current message along with the |aType| good enough?
Flags: needinfo?(vedant.sareen) → needinfo?(alessio.placitelli)
(In reply to Vedant Sareen from comment #10)
> Since we also need to mention not to submit pings, what exactly should the
> logged message be?
> Or is the current message along with the |aType| good enough?

No problem, don't worry! I was thinking about something along the lines of:

`submitExternalPing - Submission is not allowed after shutdown, discarding ping ${aType}.`
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 12

7 months ago
Created attachment 8822066 [details] [diff] [review]
Proposed patch for bug 1301311
Attachment #8821776 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8822066 [details] [diff] [review]
Proposed patch for bug 1301311

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

Overall, this looks good now, thanks. Could you add a blank line after the "Bug 130..." line in your commit message?

Please kindly address the comments below.

One last thing! When attaching your next patch, you can directly request the review instead of adding a needinfo for me: select the question mark in the reviewer's field and then start writing my name. The full name will popup!

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +495,5 @@
>      this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
>  
> +    // Reject pings sent after shutdown.
> +    if (this._shuttingDown) {
> +      this._log.error("submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: " + aType);

You can put the error message in a const variable and use it both here and in the line below.

const errorMessage = "submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: " + aType;
this._log.error(errorMessage);
return Promise.reject(new Error(errorMessage));

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +505,5 @@
> +//Testing shutdown and checking that pings sent afterwards are rejected.
> +add_task(function* test_pingRejection() {
> +  yield TelemetryController.testShutdown();
> +  yield sendPing(false, false);
> +  yield sendPing(false, true);

You can just leave one |sendPing| (whichever you prefer) and remove the others.

Also, since we're throwing an exception when sending pings after shutdown, this test should still be failing and we should assert if it doesn't.

What about changing the sendPing to

yield sendPing(false, false)
    .then(() => Assert.ok(false, "Pings submitted after shutdown must be rejected."),
          () => Assert.ok(true, "Ping submitted after shutdown correctly rejected."));
Attachment #8822066 - Flags: feedback+
Also make sure that the patch works before requesting a new review ;)
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 15

7 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #13)
 
> You can put the error message in a const variable and use it both here and
> in the line below.
> 
> const errorMessage = "submitExternalPing - Submission is not allowed after
> shutdown, discarding ping of type: " + aType;
> this._log.error(errorMessage);
> return Promise.reject(new Error(errorMessage));

Oops. Sorry, didn't realise that I had to change the reported error too.
Fixed this now :)

> Also, since we're throwing an exception when sending pings after shutdown,
> this test should still be failing and we should assert if it doesn't.
> 
> What about changing the sendPing to
> 
> yield sendPing(false, false)
>     .then(() => Assert.ok(false, "Pings submitted after shutdown must be
> rejected."),
>           () => Assert.ok(true, "Ping submitted after shutdown correctly
> rejected."));

Made these changes too.

However, when I executed the command

> ./mach test toolkit/components/telemetry/tests/unit/test_TelemetryController.js 

I got the following error message :

> 0:10.39 LOG: Thread-3 ERROR Unexpected exception Error: submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: test-ping-type at resource://gre/modules/TelemetryController.jsm:501

I noticed that the added test was nowhere in the list of passed tests (no |TEST_STATUS| corresponding to |test_pingRejection| in the output). I experimented a bit, and I think that this error has nothing to do with the added test, as I get the same error even on removing |test_pingRejection| from the file.

This was happening earlier too, before I had uploaded the initial changes file.

>Also make sure that the patch works before requesting a new review ;)

I'm having a few problems as I haven't been able to grasp what is going wrong here.

Is the exception expected? In that case, why is it so and how do I make sure that the patch is functional?

If it isn't, then again, what might be the reason behind it's occurrence and how should I proceed to fix it?

I have taken quite a lot of your time with this bug. I apologise for the delay and I thank you for your time so far. 
This has been really educational for me so far, and I certainly wish to learn more. :)
Flags: needinfo?(alessio.placitelli)
(In reply to Vedant Sareen from comment #15)
> I got the following error message :
> 
> > 0:10.39 LOG: Thread-3 ERROR Unexpected exception Error: submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: test-ping-type at resource://gre/modules/TelemetryController.jsm:501

There should be a few more lines where this occurred. There's the full stack of the rejection.
Look at where it fails in test_TelemetryController.js, it should tell you on which line.
Also, you can post the relevant portion of the log here (or attach it somewhere) so that we can have a look too!

> I noticed that the added test was nowhere in the list of passed tests (no
> |TEST_STATUS| corresponding to |test_pingRejection| in the output). I
> experimented a bit, and I think that this error has nothing to do with the
> added test, as I get the same error even on removing |test_pingRejection|
> from the file.

Your intuition might be completely right! It could be that the changes that we
added had an impact on some older test. This is unexpected, but could still be normal.
If that's the case, we should dig into the problem and find out why the other test is failing.

> This was happening earlier too, before I had uploaded the initial changes
> file.

Do you mean that the test was actually failing without *any* of your changes?

> I have taken quite a lot of your time with this bug. I apologise for the
> delay and I thank you for your time so far. 
> This has been really educational for me so far, and I certainly wish to
> learn more. :)

No problem at all, you're doing a good job there!
Flags: needinfo?(alessio.placitelli) → needinfo?(vedant.sareen)
(Assignee)

Comment 17

7 months ago
>Also, you can post the relevant portion of the log here (or attach it somewhere) so that we can have a look too!

Relevant portion of Log :

>  0:10.94 LOG: Thread-3 ERROR Unexpected exception Error: submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: test-ping-type at resource://gre/modules/TelemetryController.jsm:501
> send@resource://gre/modules/TelemetryController.jsm:501:29
> submitExternalPing@resource://gre/modules/TelemetryController.jsm:206:12
> sendPing@/home/vedant/Mozilla/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryController.js:49:10
> test_pingHasClientId@/home/vedant/Mozilla/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryController.js:220:9
> _run_next_test@/home/vedant/Mozilla/mozilla-central/testing/xpcshell/head.js:1566:9
> run@/home/vedant/Mozilla/mozilla-central/testing/xpcshell/head.js:713:9
> _do_main@/home/vedant/Mozilla/mozilla-central/testing/xpcshell/head.js:210:5
> _execute_test@/home/vedant/Mozilla/mozilla-central/testing/xpcshell/head.js:545:5
> @-e:1:1
>  0:10.94 LOG: Thread-3 INFO exiting test

Just in case I missed something of value, I am also posting the entire output as a file.

Here, we can see that the failure is at lines 49 and 220 in |test_TelemetryController.js| and lines 206 and 501 in |TelemetryController.jsm|.

Line number 220 in |test_TelemetryController.js| is a part of 
> add_task(function* test_pingHasClientId() {..});

whereas line 220 itself is
> yield sendPing(true, false);

which leads to
> function sendPing(aSendClientId, aSendEnvironment) {..}

inside which, at line 49, we have 
> return TelemetryController.submitExternalPing(TEST_PING_TYPE, {}, options);

which further calls from |TelemetryController.jsm|
> submitExternalPing: function(aType, aPayload, aOptions = {}) {..}

This is at line 202 in the file. At line number 206 in this function, we have
> return Impl.submitExternalPing(aType, aPayload, aOptions); 

which is nothing but the function in |TelemetryController.jsm| where we made our changes!

This leads us to
>  submitExternalPing: function send(aType, aPayload, aOptions) {
>    this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
> 
>    // Reject pings sent after shutdown.
>    if (this._shuttingDown) {
>      const errorMessage = "submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: " + aType;
>      this._log.error(errorMessage);
>      return Promise.reject(new Error(errorMessage));
>    }

|Promise.reject| is at line 501, which is also there in the Error Log.
Flags: needinfo?(vedant.sareen)
(Assignee)

Comment 18

7 months ago
Created attachment 8822618 [details]
Output on running |test_TelemetryController.js|
(Assignee)

Comment 19

7 months ago
>> This was happening earlier too, before I had uploaded the initial changes
>> file.
> Do you mean that the test was actually failing without *any* of your changes?

No, what I meant was that the test was failing after I had only made the changes mentioned in your initial comments, and not the extra changes made by me in which were visible in the first diff file.

I had added an extra |!this._initStarted| i.e.

>    submitExternalPing: function send(aType, aPayload, aOptions) {
>      this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
>  
> +    //Reject pings sent after shutdown
> +    if (!this._initStarted && this._shuttingDown) {

On doing so, no test fails.

However, as you mentioned earlier, this must be erroneous.
I don't think it is possible for |shuttingDown| to be set to |true| without |initStarted| being set to false. This change must have lead to some tests behaving in unexpected ways (or maybe even skipping their intended purpose), which might be why no error or exception is reported.
Vedant, would you mind attaching your latest patch to the bug? The one with the changes I've requested? Thanks!
Flags: needinfo?(vedant.sareen)
(Assignee)

Comment 21

7 months ago
Created attachment 8823623 [details] [diff] [review]
bug1301311.patch

2nd proposed patch for bug 1301311
Attachment #8822066 - Attachment is obsolete: true
Flags: needinfo?(vedant.sareen)
Attachment #8823623 - Flags: review?(alessio.placitelli)
Comment on attachment 8823623 [details] [diff] [review]
bug1301311.patch

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

Sorry for the delay, thanks for addressing the feedback from the last round.

To reset the Telemetry state between two consecutive tests, we call the reset() and the initialize the Telemetry state again. If we don't reset the |this._shuttingDown| flag as well, we won't be able to send any new pings in the test.

To do so, we need to set |this._shuttingDown = true;| right after where we're setting _initStarted [1]. Once that's done, we should be good. 

Please run the tests again as described in comment 3 and let me know if this fixes the problem.

[1] - http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/toolkit/components/telemetry/TelemetryController.jsm#660
Attachment #8823623 - Flags: review?(alessio.placitelli) → feedback+
(Assignee)

Comment 23

7 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #22)

> Sorry for the delay, thanks for addressing the feedback from the last round.

It's all right sir. Wish you a happy new year!

> To do so, we need to set |this._shuttingDown = true;| right after where
> we're setting _initStarted [1]. Once that's done, we should be good. 
> 
> Please run the tests again as described in comment 3 and let me know if this
> fixes the problem.

Surely you meant that we need to set |this._shuttingDown = false;|?
Setting it to true leads to failure on the first test itself.
Setting it to false, however, fixes the problem. All tests complete successfully on doing so.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 24

7 months ago
Created attachment 8824120 [details] [diff] [review]
3rd Proposed patch for Bug 1301311

Attachment has |this._shuttingDown = false;|
Attachment #8822618 - Attachment is obsolete: true
Attachment #8823623 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Attachment #8824120 - Flags: review?(alessio.placitelli)
Comment on attachment 8824120 [details] [diff] [review]
3rd Proposed patch for Bug 1301311

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

Vedant, thanks for the updates! Yes, I meant setting that to false, good catch. The patch looks good to me now.

@Georg, could you take a second look there?
Attachment #8824120 - Flags: review?(alessio.placitelli)
Attachment #8824120 - Flags: review+
Attachment #8824120 - Flags: feedback?(gfritzsche)
Comment on attachment 8824120 [details] [diff] [review]
3rd Proposed patch for Bug 1301311

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

Thanks for the patch, this looks good to me!

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +496,5 @@
>  
> +    // Reject pings sent after shutdown.
> +    if (this._shuttingDown) {
> +      const errorMessage = "submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: " + aType;
> +      this._log.error(errorMessage);

Nit:
Why not put the error message right here?
The separate `const errorMessage` just seems to add one line of code?
Attachment #8824120 - Flags: feedback?(gfritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] from comment #26)
> Why not put the error message right here?
> The separate `const errorMessage` just seems to add one line of code?

Never mind this, i missed that `errorMessage` is actually used in two places.

Did anyone do a simple manual test of running Firefox and checking that important pings ("main" especially) are still submitted successfully early & mid-session as well as on shutdown?
(In reply to Georg Fritzsche [:gfritzsche] from comment #27)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #26)
> > Why not put the error message right here?
> > The separate `const errorMessage` just seems to add one line of code?
> 
> Never mind this, i missed that `errorMessage` is actually used in two places.
> 
> Did anyone do a simple manual test of running Firefox and checking that
> important pings ("main" especially) are still submitted successfully early &
> mid-session as well as on shutdown?

Yes, I did, I covered the following:

- early init ping submission
- early init shutdown
- mid session ping (addon install)
- shutdown/saved-session pings on shutdown
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bef1f714505
(In reply to Vedant Sareen from comment #24)
> Created attachment 8824120 [details] [diff] [review]
> 3rd Proposed patch for Bug 1301311

Vedant, since the patch looks good, I've pushed it to our try infrastructure, as shown in comment 29. If everything looks good, we'll go on and land it! Good job!
(Assignee)

Comment 31

7 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #30)

> Vedant, since the patch looks good, I've pushed it to our try
> infrastructure, as shown in comment 29. If everything looks good, we'll go
> on and land it! Good job!

Thank you Sir! :)
It looks like this patch is triggering some failures on Android. I'm taking a look at them to see what's going on.
Priority: P3 → P1
(Assignee)

Comment 33

6 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #32)
> It looks like this patch is triggering some failures on Android. I'm taking
> a look at them to see what's going on.

Could you please tell me too about the failures which are occurring and any possible ways of replicating them, if they are known?

I'd like to help in finding out why they may be occurring and any possible solutions to them.
Flags: needinfo?(alessio.placitelli)
(In reply to Vedant Sareen from comment #33)
> (In reply to Alessio Placitelli [:Dexter] from comment #32)
> > It looks like this patch is triggering some failures on Android. I'm taking
> > a look at them to see what's going on.
> 
> Could you please tell me too about the failures which are occurring and any
> possible ways of replicating them, if they are known?

Unfortunately, this failure was particularly tricky as it was an "Android only" failure, so that's why
I dug into that.

You can take a look into the failure itself by opening the link in comment 29, clicking on one of the orange "X3" on the "Android 4.3 API15+ opt" line. After that, you can take a look at the log of the failing run by clicking on the "log" icon on the black bar at the bottom of the page, on the left.

The good news is that, after a bit, we figured out what's wrong: it has nothing to do with your patch!

A previous test [1] wasn't restoring the value of a pref after changing it. That caused the |yield TelemetryController.testReset();| I added to fix the test failure to be ineffective, as it wasn't initializing anything on Android due to that.

As a consequence, calling |yield TelemetryController.testShutdown();| in the test you added thrown an unexpected exception at [2] due to the fact that Telemetry was not initialized before.

I've updated the patch to take that in account.

Vedant, would you be interested in taking on a new bug?

[1] - http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/toolkit/components/telemetry/tests/unit/test_TelemetryController.js#428
[2] - http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/toolkit/components/telemetry/TelemetrySession.jsm#1435
Flags: needinfo?(alessio.placitelli) → needinfo?(vedant.sareen)
Created attachment 8826172 [details] [diff] [review]
bug1301311.patch

As reported in the following try push, this works ok:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=990c312970866e46c016cc6893f59a776e5bf1ee
Attachment #8824120 - Attachment is obsolete: true
Attachment #8826172 - Flags: review+
(Assignee)

Comment 36

6 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #34)
> Vedant, would you be interested in taking on a new bug?

Sure! :)
Flags: needinfo?(vedant.sareen)
Keywords: checkin-needed

Comment 37

6 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6283b785c0
Cleanly reject pings that are submitted too late in shutdown. r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7c6283b785c0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.