|
2.35 KB,
patch
|
aceman
:
review+
Jorg K (GMT+2)
:
approval-comm-aurora+
Jorg K (GMT+2)
:
approval-comm-beta+
Jorg K (GMT+2)
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170301192304
Steps to reproduce:
Select a Newsgroup
open the context menu (RMC)
Properties -> /Synchronization\
=> [Download Now]
Actual results:
TB crashes
It does not matter if the checkbox ('Select this newsgroup for offline use') is ticked.
|
(Reporter) | |
Comment 1•5 months ago
|
||
Comm-Central: last good Changeset: 21218 (5f72602f1ff0) Bug 1337865 - Force release of subserver's listener. r=aceman first bat Changeset: 21219 (ef7420896e68) Bug 1337865 - fix some leaking objects in mailnews/news. r=aceman
|
(Assignee) | |
Comment 2•5 months ago
|
||
Created attachment 8846080 [details] [diff] [review] 1346338-fix-crash.patch Thanks for reporting this and thanks for being on the Daily/Aurora channel where the ride can be bumpy, as we see here. Aceman, please keep in mind that 'new' is infallible in Mozilla, if the memory cannot be allocated, control doesn't return to the caller.
|
|
(Assignee) | |
Updated•5 months ago
|
||
There is no crash report in this bug. So how does the fix work? How did we cause this in bug 1337865 ?
|
|
(Assignee) | |
Comment 5•5 months ago
|
||
Oh, I thought this was a simple review :-( OK, no crash report, but the crash is easily reproducible, just follow the steps. How did we cause this. Easy. There was a new object allocated, which was clearly leaking. So I added a 'delete' in the function that created it. Obviously though, someone else grabbed a reference to the object which was dereferenced later/elsewhere/asynchronously/who-knows. So the obvious Mozilla fix is letting ref-counting do its magic. The object will magically die when the last interested party releases the reference.
Comment on attachment 8846080 [details] [diff] [review] 1346338-fix-crash.patch Review of attachment 8846080 [details] [diff] [review]: ----------------------------------------------------------------- OK, let's try it. But I do not see how can somebody outside keep reference to downloadState .
|
|
(Assignee) | |
Comment 7•5 months ago
|
||
It's invisible, like all the workings of software. But seriously: downloadState->DownloadArticles(...) calls nsNewsDownloader::DownloadArticles(). In there we set a bunch of member variables, call DownloadNext(true) and return. Upon return the object is destroyed, way before the async download is finished. Surely I should have know this before causing the crash ;-(
|
|
(Assignee) | |
Comment 8•5 months ago
|
||
https://hg.mozilla.org/comm-central/rev/a10e7c19d7f373693e2a193204f17b4de3f7f741
|
|
(Assignee) | |
Comment 9•5 months ago
|
||
Comment on attachment 8846080 [details] [diff] [review] 1346338-fix-crash.patch We'll need to fix the crash in Aurora as well. This will go to ESR 52 together with bug 1337865 in due course.
|
|
(Assignee) | |
Comment 10•5 months ago
|
||
Aurora (TB 54): https://hg.mozilla.org/releases/comm-aurora/rev/39eaa29525624f85ed1bc0fdb7e71be54d4c2b06 Crash will be fixed in Earlybird and Daily tomorrow. Thanks again for reporting it.
|
|
(Assignee) | |
Updated•5 months ago
|
||
|
|
(Assignee) | |
Updated•5 months ago
|
||
|
|
(Assignee) | |
Comment 11•4 months ago
|
||
Beta (TB 53): https://hg.mozilla.org/releases/comm-beta/rev/27fd17b01040de8023be9ec3ef4b979d4939b16d
|
|
(Assignee) | |
Comment 12•3 months ago
|
||
TB 52 ESR: https://hg.mozilla.org/releases/comm-esr52/rev/ac959d841e939551b5c944b6113110d15218a29d
|
|
(Assignee) | |
Updated•3 months ago
|
||
Description
•