299168 - misuse of nsISafeOutputStream

Status: RESOLVED INVALID

(Core :: Networking: File, defect)

Description

[[e-mail]]

followup from bug 295732 comment 25: all the js code in toolkit that uses nsISOS
does stuff like

var fos =
Components.classes["@mozilla.org/network/safe-file-output-stream;1"].createInstance(Components.interfaces.nsIFileOutputStream);
{...}
fos.finish();

without ever QI'ing the stream to nsISOS. i think that will throw an XPC error,
since XPC doesn't know the instance even supports that iface. which is probably
why ben wrapped all the finish() calls with try/catches.

i don't know the machinery of XPC very well, so if anyone can contradict that,
please do ;)

Comment 1

[[e-mail]]

occurrences:

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#459
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/update/src/nsUpdateService.js.in#224

also, i'm not sure why we create the file first, e.g.:
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/update/src/nsUpdateService.js.in#370

but i'm not as worried about that.

Comment 2

[[e-mail]]

Attachment: use qi

something like this. timeless, mind taking a look?

Comment 3

[Darin Fisher]

Comment on attachment 187695 [details] [diff] [review]
use qi

-  if (stream instanceof Components.interfaces.nsISafeOutputStream)
-    stream.finish();
-  else
-    stream.close();

unless i'm mistaken, this code adds properties from nsISafeOutputStream onto
stream.  instanceof is just another way of doing QueryInterface, so i don't see
how this patch changes anything.

Comment 4

[timeless]

Comment on attachment 187695 [details] [diff] [review]
use qi

>-  if (stream instanceof Components.interfaces.nsISafeOutputStream)
in general this causes an implicit QI to the interface and stream is annotated
to include a pointer to the interface, flattening then enables you to call
methods of that interface.

>+    var sos = fos.QueryInterface(Components.interfaces.nsISafeOutputStream);

(note that the code you're replacing calls close() in the else case, your new
code doesn't do anything.) i don't believe you've found the right problem
source.

Comment 5

[Darin Fisher]

Here's a little xpcshell session to demonstrate what I'm talking about:

js> var f =
Components.classes["@mozilla.org/network/safe-file-output-stream;1"].createInstance();
js> f
[xpconnect wrapped nsISupports @ 0x8167a60 (native @ 0x8167a28)]
js> for (p in f) dump(p + "\n");
QueryInterface
js> f instanceof Components.interfaces.nsISafeOutputStream;
true
js> for (p in f) dump(p + "\n");
QueryInterface
finish

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187695 [details] [diff] [review]
use qi

-  if (stream instanceof Components.interfaces.nsISafeOutputStream)

this is a QI in disguise, I don't think this patch is needed.

Comment 7

[Robert Strong (they/them - no direct email)]

mozillafly - are you still able to reproduce this error? As I recall, it
depended on the version of gcc you were using.

Comment 8

[[e-mail]]

(In reply to comment #6)
> this is a QI in disguise, I don't think this patch is needed.

yeap, gotcha... thanks.

agreed that the problem must be something else, no need for this bug.

Comment 9

[bugzilla]

(In reply to comment #7)
> mozillafly - are you still able to reproduce this error? As I recall, it
> depended on the version of gcc you were using.

I have recently done builds with gcc 3.3.6 and gcc 4.0.1 and am not able to
reproduce the error I was seeing before.