824581 - RemoteOpenFileChild::AsyncRemoteFileOpen crashes on Windows/Mac

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Bobby Holley (:bholley)]

Trying to do an OOP b2g desktop build seems fraught with peril, but this is the first thing that breaks. JDM suggested the patch.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Bug 824581 - Avoid a null deref in the ifdef. v1

Comment 2

[Jason Duell]

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!

Comment 3

[Bobby Holley (:bholley)]

Attachment: Avoid a null deref in the ifdef. v2 r=jduell

Comment 4

[Bobby Holley (:bholley)]

Attachment: Stop skipping tests on window. v1

Comment 5

[Bobby Holley (:bholley)]

(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 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 695793 [details] [diff] [review]
Avoid a null deref in the ifdef. v2 r=jduell

Err, wrong patch. The other is obsolete.

Comment 8

[Bobby Holley (:bholley)]

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.

Comment 9

[Jason Duell]

Attachment: v3

also removed nulling out mListener (we never set it)

Comment 10

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/418baef1894a
https://hg.mozilla.org/releases/mozilla-aurora/rev/54034e3cc092
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2c37129a8086

One-liner fix to patch that's already a+ for aurora/b2g18, so marked a=b2gbustage.

Comment 11

[Jason Duell]

whoops, landed on inbound, not m-c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/418baef1894a