302227 - Modal dialog run from onStateChange(STATE_START) may crash

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect, P5)

Description

[Darin Fisher]

Modal dialog run from onStateChange(STATE_START) may crash.

The problem occurs because of code like this in AsyncOpen implementations:

  NS_IMETHODIMP nsFooChannel::AsyncOpen(... listener) {
    ...
    mStreamPump->AsyncRead(this);
    ...
    mLoadGroup->AddRequest(this);
    
    mListener = listener;
    return NS_OK;
  }
  NS_IMETHODIMP nsFooChannel::OnStartRequest(...) {
    return mListener->OnStartRequest(...);
  }

The AddRequest call on the LoadGroup object causes onStateChange(STATE_START) to
be sent to WebProgressListener implementations.  If any WPL calls window.alert,
then it will cause ProcessPendingEvents to be run, which may cause events from
nsFooChannel's mStreamPump to run before AddRequest returns.  This may cause
nsFooChannel's OnStartRequest method to run, which would crash because mListener
is not yet initialized.

This is definitely a problem with nsFileChannel, and it is likely a problem with
other channels as well.  I for one do not like APIs like this that result in
channel code being re-entered before AsyncOpen returns because that makes
implementing AsyncOpen very tricky.  Remember: AsyncOpen needs to only store
mListener if it is going to return NS_OK because setting mListener may result in
a reference cycle that will only be broken with OnStopRequest runs.  So, solving
this bug is tricky.  I really wish that onStateChange(STATE_START) were an
asynchronous event instead, but I'm not sure if we can make such a change.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We could document that modal dialogs are not allowed from onStateChange... We
already have such restrictions on other apis (eg nsIContentPolicy).

Comment 2

[Darin Fisher]

*** Bug 304259 has been marked as a duplicate of this bug. ***

Comment 3

[timeless]

it won't work, see duplicate.

document that objects should not call out to arbitrary xpcom code in an 
inconsistent state. and after documenting it, fix mozilla to honor the doc :)

i think i can find some people to work on that.

Comment 4

[Darin Fisher]

My plan is two-fold:

1) Re-arrange assignment of mListener so that if we do execute OnStartRequest
before AddRequest returns, we at least won't crash in Necko.

2) Document that modal dialogs cannot be shown in onStateChange(STATE_START). 
I'm not sure about other states.

Long-term we should fix event queues to not process the events of an elder queue
when popped.

Comment 5

[Darin Fisher]

Attachment: partial fix for some of the protocol handlers

Comment 6

[timeless]

> 2) Document that modal dialogs cannot be shown in onStateChange(STATE_START). 
> I'm not sure about other states.

i'll repeat. you can't do this. venkman will push a nested event queue to debug
js called in onStateChange

Comment 7

[Darin Fisher]

> i'll repeat. you can't do this. venkman will push a nested event queue to debug
> js called in onStateChange

OK.  that's good to know.  sorry, i don't recall where you had mentioned venkman
before.  i think we will not solve this bug completely for gecko 1.8.  for gecko
1.9, my plan is still to make it so that nested event queues do not process
events targeted at a parent event queue when they are popped.

Comment 8

[timeless]

comment 3 "it won't work, see duplicate" Bug 304259 comment 0

Comment 9

[Darin Fisher]

(In reply to comment #8)
> comment 3 "it won't work, see duplicate" Bug 304259 comment 0

there are no references to venkman (that i can see) in that bug either, but ok.

Comment 10

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

(bug 254151 comment 0, fwiw)

Comment 11

[timeless]

Bug 304259 comment 0 	jsd3250.dll!jsdService::EnterNestedEventLoop
bug 254151 comment 7

Comment 12

[timeless]

this is needed for the 1.8 branch, not having it means that using venkman with 
any code that interacts with channels is likely to result in crashes.

Comment 13

[Darin Fisher]

Even with this patch (or the complete one), you're still likely to run into
trouble using venkman that way.  The problem is that this will still setup odd
re-entrancy conditions on consumes of Necko.  So, even if Necko is okay with the
odd re-entrancy, it's consumers may not.  And, due to the fact that events in
elder queues are processed when popping a younger queue, there is no good way
for Necko to prevent said re-entrancy from occuring.  The best solution is to
fix PopThreadEventQueue to not ProcessPendingEvents on all queues, but that is
likely to have other implications, which would be way too risky to entertain on
the 1.8 branch.

Comment 14

[timeless]

having something would be better than what we have now, and if we hit other 
things, we'll report them.

Comment 15

[Asa Dotzler [:asa]]

not gonna make the 1.5 train. maybe in a dot release if we come up with a
low-risk fix that actually solves the venkman problems.

Comment 16

[timeless]

this is still a big problem especially when we need to find out why progress meters don't end up at done.

Comment 17

[Darin Fisher]

-> defaults

Comment 18

[Benjamin Smedberg]

Not going to block on this, but we'd love a patch.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Benjamin, I thought we wanted to at least block on API documentation that says that extensions shouldn't do this?

Comment 20

[Benjamin Smedberg]

I wouldn't consider this a stop-ship, no.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Sure, but I still think the docs are wanted.

Comment 22

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 23

[Andrei Purice]

Marking this as Resolved > Incomplete since the reporter's email has been deactivated and there's no way of asking for a repro.
If anyone can still reproduce this issue please re-open this issue or file a new one.