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)
Toolkit Graveyard
OS.File
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)
1.88 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•9 years ago
|
||
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.
Reporter | ||
Comment 3•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(dteller)
Reporter | ||
Comment 5•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
@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•9 years ago
|
||
Or you may get started with mercurial with https://developer.mozilla.org/en/docs/Mercurial_Queues
Reporter | ||
Comment 9•9 years ago
|
||
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)
Reporter | ||
Comment 10•9 years ago
|
||
Hi Aakash, how are things going?
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8554927 -
Flags: review?(dteller)
Reporter | ||
Comment 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
(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
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8557461 -
Flags: review?(dteller)
Reporter | ||
Comment 16•9 years ago
|
||
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.
Reporter | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a954002b3be
Reporter | ||
Comment 18•9 years ago
|
||
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)
Reporter | ||
Comment 19•9 years ago
|
||
Tests look good. Are you ready to land the patch?
Assignee | ||
Comment 20•9 years ago
|
||
yes , but should I make a new patch or you will commit the prevoius patch I am sorry for misunderstanding
Reporter | ||
Comment 21•9 years ago
|
||
No need, let's just land this.
Flags: needinfo?(aakashshah512)
Keywords: checkin-needed
Comment 22•9 years ago
|
||
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)
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 23•9 years ago
|
||
Attachment #8563338 -
Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8575247 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 24•9 years ago
|
||
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
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla39
Updated•11 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•