Closed Bug 824581 Opened 8 years ago Closed 8 years ago

RemoteOpenFileChild::AsyncRemoteFileOpen crashes on Windows/Mac

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
b2g18 --- fixed

People

(Reporter: bholley, Assigned: jduell.mcbugs)

References

Details

Attachments

(1 file, 3 obsolete files)

Trying to do an OOP b2g desktop build seems fraught with peril, but this is the first thing that breaks. JDM suggested the patch.
Attachment #695570 - Flags: review?(jduell.mcbugs)
Comment on attachment 695570 [details] [diff] [review]
Bug 824581 - Avoid a null deref in the ifdef. v1

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

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +114,2 @@
>    mListener->OnRemoteFileOpenComplete(NS_OK);
>    mListener = nullptr;

Yeah, this is a definite bug.  +r if you change the code to just call aListener->OnRemoteFileOpenComplete and drop the lines that set/unset mListener.

Extra bonus points if you're willing to run this through try (I'm away from a setup that lets me push to try right now) with the xpcshell.ini line here

  https://hg.mozilla.org/releases/mozilla-aurora/rev/e364c2c32501

changed to skip only OSX. (you can tell try to only run xpcshell tests).  I suspect you've just fixed the reason we needed to skip that test on Windows.  If so, please roll that change into this patch, too.

And please mark the new patch aurora? and b2g18?--we'll want it on those branches.

Thanks!
Attachment #695570 - Flags: review?(jduell.mcbugs) → review+
Attachment #695793 - Flags: review+
(In reply to Jason Duell (:jduell) from comment #2)

> Yeah, this is a definite bug.  +r if you change the code to just call
> aListener->OnRemoteFileOpenComplete and drop the lines that set/unset
> mListener.

Done.


> Extra bonus points if you're willing to run this through try (I'm away from
> a setup that lets me push to try right now) with the xpcshell.ini line here
> 
>   https://hg.mozilla.org/releases/mozilla-aurora/rev/e364c2c32501

https://tbpl.mozilla.org/?tree=Try&rev=70fdd7190809
Comment on attachment 695570 [details] [diff] [review]
Bug 824581 - Avoid a null deref in the ifdef. v1

Flagging for approval per comment 2. This just fixes a dumb null-deref inside of an #ifdef. Risk free.
Attachment #695570 - Flags: approval-mozilla-b2g18?
Attachment #695570 - Flags: approval-mozilla-aurora?
Comment on attachment 695793 [details] [diff] [review]
Avoid a null deref in the ifdef. v2 r=jduell

Err, wrong patch. The other is obsolete.
Attachment #695793 - Flags: approval-mozilla-b2g18?
Attachment #695793 - Flags: approval-mozilla-aurora?
Attachment #695570 - Attachment is obsolete: true
Attachment #695570 - Flags: approval-mozilla-b2g18?
Attachment #695570 - Flags: approval-mozilla-aurora?
Comment on attachment 695795 [details] [diff] [review]
Stop skipping tests on window. v1

Looks like the try is green. Lets get this checked in too.
Attachment #695795 - Flags: review+
Keywords: checkin-needed
Attached patch v3Splinter Review
also removed nulling out mListener (we never set it)
Assignee: bobbyholley+bmo → jduell.mcbugs
Attachment #695793 - Attachment is obsolete: true
Attachment #695795 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #695793 - Flags: approval-mozilla-b2g18?
Attachment #695793 - Flags: approval-mozilla-aurora?
Attachment #696770 - Flags: review+
whoops, landed on inbound, not m-c
https://hg.mozilla.org/mozilla-central/rev/418baef1894a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.