the http-on-response-set-cookie notification added in bug 970246 is right now being dispatched to main thread (from the main thread itself!) . this causes issues like https://bugzilla.mozilla.org/show_bug.cgi?id=970517#c69 After talking with mayhemer, he suggested to make them sync instead of dispatching.

Shian-yao: this could be a good bug for someone in Taipei (probably Amy but I'll leave that up to you).

Thanks Jason. Amy will take this bug.

Hi Girish, I found I didn't occur the error like https://bugzilla.mozilla.org/show_bug.cgi?id=970517#c69, bug I still modified dispatch to sync and the patch on attachment. Would you give me some suggestions, or do I need to modify it? Thanks!

Comment on attachment 8789260 [details] [diff] [review] bug-1013635.patch Hi Honza, I found I can't reproduce the situation from https://bugzilla.mozilla.org/show_bug.cgi?id=970517#c69, and I already tested on firefox 34.0 and 35.0 for reproducing the error situation. For this reason, Do I have to modify dispatch to sync like my attachment? Thanks!

Comment on attachment 8789260 [details] [diff] [review] bug-1013635.patch Review of attachment 8789260 [details] [diff] [review]: ----------------------------------------------------------------- No no. What I was suggesting was to call the callback directly from here, no dispatch at all. The NS_DISPATCH_SYNC flag is something extremely dangerous to use (IMHO should be removed from the platform). It can lead to re-entrances and unexpected and very hard to fix issues. The re-dispatch in the current code is there because I simply didn't want the caller of SetCookie be blocked by a JS callback execution. But that might be a too cautious approach and actually causes trouble. Still, a change to call the callback directly is something to keep an eye on after it lands. There is some small chance it may cause some trouble, either in Necko or in devtools.

Hi Honza, I have added a notify function for instead of of dispatch. Please help me to review my patch, thanks!

Comment on attachment 8790228 [details] [diff] [review] Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits. Review of attachment 8790228 [details] [diff] [review]: ----------------------------------------------------------------- this is it. please push to try and make sure devtools work with this as before AND that the bug with null loadgroup has been fixed. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +2022,5 @@ > return NS_OK; > } > > +void > +HttpBaseChannel::NotifyCookieRunnable(char const *aCookie) { open brace on a new line please @@ +2023,5 @@ > } > > +void > +HttpBaseChannel::NotifyCookieRunnable(char const *aCookie) { > + nsString mCookie; use nsAutoString cookie; instead. @@ +2024,5 @@ > > +void > +HttpBaseChannel::NotifyCookieRunnable(char const *aCookie) { > + nsString mCookie; > + CopyASCIItoUTF16(aCookie, mCookie); maybe do this conversion only when there is the observer service (inside the if (obs) branch.) ::: netwerk/protocol/http/HttpBaseChannel.h @@ +340,5 @@ > > + // This is fired only when a cookie is created due to the presence of > + // Set-Cookie header in the response header of any network request. > + // This notification will come only after the "http-on-examine-response" > + // is fired, but it will not always appear. I think the "but it will not always appear." part is redundant (and confusing). @@ +341,5 @@ > + // This is fired only when a cookie is created due to the presence of > + // Set-Cookie header in the response header of any network request. > + // This notification will come only after the "http-on-examine-response" > + // is fired, but it will not always appear. > + void NotifyCookieRunnable(char const *aCookie); A better name would be "NotifySetCookie". There is no longer any runnable involved here.

Hi Honza, I have modified code from your suggestions, and pushed the patch to try server. https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa837ed6692b&selectedJob=27367699 According to the testing result, the patch doesn't cause loadgroup to null. Please help me to review my patch, thanks!

Comment on attachment 8790841 [details] [diff] [review] Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits. Review of attachment 8790841 [details] [diff] [review]: ----------------------------------------------------------------- thanks. try looks OK. ship it! (just update the commit message before landing) ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +2027,4 @@ > { > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + if (obs) { > + nsAutoString mCookie; nit: nsAutoString cookie; the "m" prefix is only on members. ::: netwerk/protocol/http/HttpBaseChannel.h @@ +340,5 @@ > > + // This is fired only when a cookie is created due to the presence of > + // Set-Cookie header in the response header of any network request. > + // This notification will come only after the "http-on-examine-response" > + // is fired. nit: was fired.

Comment on attachment 8792759 [details] [diff] [review] Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits. Review of attachment 8792759 [details] [diff] [review]: ----------------------------------------------------------------- modify by reviewer's comment, carry r+

Comment on attachment 8792759 [details] [diff] [review] Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits. Review of attachment 8792759 [details] [diff] [review]: ----------------------------------------------------------------- try: -b do -p all -u all -t none

Comment on attachment 8792759 [details] [diff] [review] Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits. Review of attachment 8792759 [details] [diff] [review]: ----------------------------------------------------------------- -b do -p all -u all -t none

Comment on attachment 8792759 [details] [diff] [review] Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits. Review of attachment 8792759 [details] [diff] [review]: ----------------------------------------------------------------- Bug 1013635 - Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits, r=honzab

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4da181c226 Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits, r=honzab