Closed Bug 1106938 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit Graveyard :: OS.File, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: Yoric, Assigned: mohamedwaleed2012, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 4 obsolete files)

      No description provided.
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]
Mentor: dteller
Whiteboard: [lang=js][good first bug]
hello Yoric,
I'm interested in working on this bug. I'm new to Bugzilla. please help me on how to get started.
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.
yes
Flags: needinfo?(dteller)
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
Flags: needinfo?(dteller)
Attached file corrected code (obsolete) —
I'm sending the code
@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...
Or you may get started with mercurial with
https://developer.mozilla.org/en/docs/Mercurial_Queues
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.
Flags: needinfo?(aakashshah512)
Hi Aakash, how are things going?
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
Attached patch 0001-Bug-1106938.patch (obsolete) — Splinter Review
Attachment #8554927 - Flags: review?(dteller)
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.
Attachment #8554927 - Flags: review?(dteller) → feedback+
(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
Attached patch 0002-Bug-1106938.patch (obsolete) — Splinter Review
Attachment #8557461 - Flags: review?(dteller)
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.
Attached patch Merged patch (obsolete) — Splinter Review
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.
Attachment #8541851 - Attachment is obsolete: true
Attachment #8554927 - Attachment is obsolete: true
Attachment #8557461 - Attachment is obsolete: true
Attachment #8557461 - Flags: review?(dteller)
Tests look good. Are you ready to land the patch?
yes , but should I make a new patch or you will commit the prevoius patch 
I am sorry for misunderstanding
No need, let's just land this.
Flags: needinfo?(aakashshah512)
Keywords: checkin-needed
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
Flags: needinfo?(dteller)
Attached patch UnbitrottenSplinter Review
Attachment #8563338 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8575247 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/13cb4d5ea178
Assignee: nobody → mohamedwaleed2012
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/13cb4d5ea178
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla39
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: