1106938 - [OS.File] When we store information on calls for AsyncShutdown, we could store the Task.stack

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

We have a few known cases in which OS.File cannot shutdown properly, and we have no clue why. To help us debug the issue, it would be interesting to have the stack of calls that trigger this situation.

The source lives here: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm?from=osfile_async_front.jsm#302

To fix this bug, it should be sufficient to change the line in which we assign
  Scheduler.latestSent = [Date.now(), ...message]
and replace it with
  Scheduler.latestSent = [Date.now(), Task.Debugging.generateReadableStack(new Error().stack), ...message]

Comment 2

[aakash]

hello Yoric,
I'm interested in working on this bug. I'm new to Bugzilla. please help me on how to get started.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Hi aakash. Thanks for your interest. Have you already downloaded and built Firefox from the source code? If not, you can find the instructions here: https://developer.mozilla.org/fr/docs/Simple_Firefox_build

By the way, the best way to make sure that I read your message is to use "Need more information from" with my name on the bottom of the Bugzilla page.

Comment 4

[aakash]

yes

Comment 5

[David Teller [:Yoric] - still alive but not very active]

In that case, you should:
1. apply the instructions in comment 1;
2. prepare a patch;
3. attach the patch to this bug using link "Add an attachment", and mark it for review by me.

If you have difficulties with any of these steps, the best place to ask questions is over irc:

http://client00.chat.mibbit.com/?server=irc.mozilla.org&channel=%23introduction&nick=aakash

Comment 6

[aakash]

Attachment: corrected code

I'm sending the code

Comment 7

[Amod [:AMoz]]

@Aakash, hi you need to create a patch file and not a text file. As I had shown you personally on how to create diff file. Refer
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch       for more details. 

@Yoric. Sorry for interfering... Actually I got aakash started by directing him to this bug.... So thought not to trouble you for Petty reasons... Hence commented...

Comment 8

[Amod [:AMoz]]

Or you may get started with mercurial with
https://developer.mozilla.org/en/docs/Mercurial_Queues

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Aakash, Amod is right, you need to attach a patch, otherwise we will not be able to do anything with it. You can follow Amod's instructions, or come over to IRC if you need further directions.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Hi Aakash, how are things going?

Comment 11

[Mohamed Waleed]

hello Yoric,
It is my first time to participating in bugzilla , I built the repo and run it 
Could you assign me to this bug , i will provid the patch file for this bug as you mentioned in comment 1

Comment 12

[Mohamed Waleed]

Attachment: 0001-Bug-1106938.patch

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8554927 [details] [diff] [review]
0001-Bug-1106938.patch

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

This looks good with a minor change below.

Also, please make sure to change your commit message to something along the lines of
Bug 1106938 - When OS.File stores information on calls for AsyncShutdown, also store the Task.stack;r=yoric

Do you have try access to test your change?

Note: I'm currently sick and not very responsive. Apologies if I make you wait.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +299,4 @@
>  
>        try {
>          Scheduler.latestReceived = [];
> +        Scheduler.latestSent = [Date.now(), Task.Debugging.generateReadableStack(new Error().stack), ...message]

Nits: That line is too long, could you cut it? Also, please add a `;` at the end of the line.

Comment 14

[Mohamed Waleed]

(In reply to David Rajchenbach-Teller [:Yoric] (away until Febrary - use "needinfo") from comment #13)
> Comment on attachment 8554927 [details] [diff] [review]
> 0001-Bug-1106938.patch
> 
> Review of attachment 8554927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good with a minor change below.
> 
> Also, please make sure to change your commit message to something along the
> lines of
> Bug 1106938 - When OS.File stores information on calls for AsyncShutdown,
> also store the Task.stack;r=yoric
> 
> Do you have try access to test your change?
> 
> Note: I'm currently sick and not very responsive. Apologies if I make you
> wait.
> 
> ::: toolkit/components/osfile/modules/osfile_async_front.jsm
> @@ +299,4 @@
> >  
> >        try {
> >          Scheduler.latestReceived = [];
> > +        Scheduler.latestSent = [Date.now(), Task.Debugging.generateReadableStack(new Error().stack), ...message]
> 
> Nits: That line is too long, could you cut it? Also, please add a `;` at the
> end of the line.


 Do you have try access to test your change?
Do you mean that I run the unit test after i made my change , if yes , I run it and tests pass
or you mean that I should write unit test for this change of code

generally I will upload the new patch for review as you mentioned in the previous review

Comment 15

[Mohamed Waleed]

Attachment: 0002-Bug-1106938.patch

Comment 16

[David Teller [:Yoric] - still alive but not very active]

The Try server is a way to run all unit tests on all platforms. Once you have uploaded your final patch, I will run the tests on the Try server. Once we have landed the patch, I will vouch for you if you wish to request access to the Try server.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a954002b3be

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: Merged patch

I have merged your patch and fixed the commit message.
The Try results will appear here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=355b3d9dec64

Sorry for the delay, I was sick.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Tests look good. Are you ready to land the patch?

Comment 20

[Mohamed Waleed]

yes , but should I make a new patch or you will commit the prevoius patch 
I am sorry for misunderstanding

Comment 21

[David Teller [:Yoric] - still alive but not very active]

No need, let's just land this.

Comment 22

[Carsten Book [:Tomcat]]

this failed to apply:

Hunk #1 FAILED at 293
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/osfile/modules/osfile_async_front.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh import.diff

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unbitrotten

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/13cb4d5ea178