1013635 - http-on-response-set-cookie notification should be sync

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Girish Sharma [:Optimizer]]

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.

Comment 1

[Jason Duell]

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

Comment 2

[Shian-Yow Wu [:swu]]

Thanks Jason.  Amy will take this bug.

Comment 3

[Amy Chung [:Amy]]

Attachment: bug-1013635.patch

Comment 4

[Amy Chung [:Amy]]

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 5

[Amy Chung [:Amy]]

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 6

[Honza Bambas (:mayhemer)]

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.

Comment 7

[Amy Chung [:Amy]]

Attachment: Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits.

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

Comment 8

[Honza Bambas (:mayhemer)]

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.

Comment 9

[Amy Chung [:Amy]]

Attachment: Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits.

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 10

[Honza Bambas (:mayhemer)]

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 11

[Amy Chung [:Amy]]

Attachment: Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43976c9364b9&selectedJob=27643283

Comment 12

[Amy Chung [:Amy]]

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 13

[Amy Chung [:Amy]]

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 14

[Amy Chung [:Amy]]

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 15

[Amy Chung [:Amy]]

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

Comment 16

[Amy Chung [:Amy]]

Attachment: Removed the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43976c9364b9&selectedJob=27643283

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f4da181c226