Closed Bug 1013635 Opened 6 years ago Closed 3 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Optimizer, Assigned: amchung)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 4 obsolete files)

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.
Assignee: scrapmachines → mcmanus
Whiteboard: [necko-active]
Shian-yao:  this could be a good bug for someone in Taipei (probably Amy but I'll leave that up to you).
Assignee: mcmanus → swu
Flags: needinfo?(swu)
Thanks Jason.  Amy will take this bug.
Assignee: swu → amchung
Flags: needinfo?(swu)
Attached patch bug-1013635.patch (obsolete) — Splinter Review
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!
Flags: needinfo?(scrapmachines)
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!
Attachment #8789260 - Flags: feedback?(honzab.moz)
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.
Attachment #8789260 - Flags: feedback?(honzab.moz) → feedback-
Hi Honza,
I have added a notify function for instead of of dispatch.
Please help me to review my patch, thanks!
Attachment #8789260 - Attachment is obsolete: true
Attachment #8790228 - Flags: feedback?(honzab.moz)
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.
Attachment #8790228 - Flags: feedback?(honzab.moz) → feedback+
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!
Attachment #8790228 - Attachment is obsolete: true
Attachment #8790841 - Flags: review?(honzab.moz)
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.
Attachment #8790841 - Flags: review?(honzab.moz) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Flags: needinfo?(scrapmachines)
You need to log in before you can comment on or make changes to this bug.