Need to implement nsIProcess (blocking feature) on the Mac

VERIFIED FIXED in mozilla0.9

Status

()

Core
XPCOM
P2
major
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: dbragg, Assigned: Samir Gehani)

Tracking

Trunk
mozilla0.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [smartupdate] Fix in hand)

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
The nsIProcess implementation on the Mac(xpcom/threads/nsProcessMac.cpp)
 currently ignores the blocking boolean.  Current behavior: executes another
process and immediately returns.

Assigning to dougt as module owner and per discussion with him.

Updated

17 years ago
Keywords: helpwanted
Target Milestone: --- → Future
Sorry, can't Future. This feature is required to be able to successfully 
install plugins via XPInstall without requiring a browser restart, which is in 
turn required by at least two major Mozilla clients.  If you can't/won't do 
this please bounce it up through your manager and let them fight it out who 
should own this.
Keywords: helpwanted → nsbeta1
Target Milestone: Future → ---

Comment 2

17 years ago
dan, if you need it, you implement it.  I don't remember why this was not
implmented in the first place.  
Assignee: dougt → dveditz
(Reporter)

Comment 3

17 years ago
I'm afraid I don't understand you're last comment Doug.  We had a discussion
about this before I created the bug and assigned it to you.  At the time you
were willing to do the implementation.  In fact, you only need to do 1/2 the
implementation because the non-blocking case is already in nsProcessMac.cpp.
All we need is the blocking case.
We desperately need a mac person to help us out. sdagley pointed us toward
watching based on the launched process's serial number.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [smartupdate]
(Assignee)

Comment 5

17 years ago
I'll implement this and consult sdagley if needed.  We may need this sooner than
anticipated for a third party installer that may get added.
Assignee: dveditz → sgehani
Priority: -- → P2
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 6

17 years ago
Created attachment 28871 [details] [diff] [review]
Loop with a sleep until the process is no longer the process manager's list if blocking == true.
(Assignee)

Comment 7

17 years ago
sdagley, please review.
sfraser, please super review.

Thanks.
Status: NEW → ASSIGNED
Whiteboard: [smartupdate] → [smartupdate] Fix in hand

Comment 8

17 years ago
Looks good, but does it work?
Minor changes:
1. I think 50ms is too short a sleep time here, since (I guess) the app that
   runs is going to take 1 second or more. Try 200ms, and tune down if it
   feels too sluggish.

2. I'd like to see an assertion hit if this is called on the main thread.
(Assignee)

Comment 9

17 years ago
Yes, Simon, this works.  I tested it through XPInstall.

I'll change the sleep value to 200ms.  Although the app didn't feel sluggish
with 50ms too on my G3 400MHz machine.  I'll go with 200ms to accomodate lower
end machines and maybe QA can advise on those lower end configs if they feel the
app is visibly sluggish.

Regarding the assertion: I looked around and consulted dveditz but there is no
low cost way to find whether we are in fact the UI thread that we could find.
If you know of a way to do this please advise.  Besides, dveditz's point was
that this would be pretty obvious when someone tested this if it was called on
the UI thread since the app wouldn't launch.
(Assignee)

Comment 10

17 years ago
Created attachment 28927 [details] [diff] [review]
Revised patch with 200ms sleep.
The app would launch, but Mozilla would hang until it's done. Sorry if I was 
unclear. I still think it'd be pretty obvious to anyone who tried to do that, 
but if anyone knows a cheap way to figure out if you're on the UI thread an 
assertion would be nice.
(Assignee)

Comment 12

17 years ago
Dan,
Actually on the Mac the app would not launch because WaitNextEvent() would not
be called since the main event loop would never be reached after we sit in the
sleep loop on the UI thread itself.  It is required that either of two Event
Manager routines be called: WaitNextEvent() or EventAvail() for an app to launch
after calling the Process Manager's LaunchApplication routine.  I guess I was
the one who was unclear.
(Assignee)

Comment 13

17 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 14

17 years ago
Build: 2001-04-03-09-trunk(MAC)

This is now working for me.  A couple of blocking test cases for Mac are 
http://jimbob/jars/a_exe_2_mac_block_true.xpi and 
http://jimbob/jars/a_filop_exe_2_mac_block_t.xpi

When blocking is set to false, it behaves as expected.

Changing QA Contact to me.  Marking Verified!
Status: RESOLVED → VERIFIED
QA Contact: kandrot → jimmylee
You need to log in before you can comment on or make changes to this bug.