Closed Bug 1293067 Opened 9 years ago Closed 5 years ago

(feature request): Implement a cookie limit warning

Categories

(Core :: Networking: Cookies, enhancement, P3)

Product:
Core
Component:
Networking: Cookies
Version:
48 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla76
Tracking Flags:
Tracking Status
firefox76 --- fixed

People

(Reporter: fabiosantosart, Assigned: baku)

Details

(Whiteboard: [necko-backlog])

Attachments

(6 files, 6 obsolete files)

Added a Notify funtion when cookie oversize.
9.95 KB, patch
Details | Diff | Splinter Review
Added a error msg when setting cookie and length of cookie is oversize.
4.93 KB, patch
Details | Diff | Splinter Review
Part2: test cases.
4.81 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part1: implementation: Added a ReportToConsole on CookieService.
9.54 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Bug 1293067 - Implement a cookie limit warning, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1293067 - Implement a cookie limit warning - tests, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0 Build ID: 20160728204513 Steps to reproduce: When you create a cookie which is too long to be registered in document.cookie, like so: > document.cookie = 'foo=' + Array(4097).join('x') Actual results: The cookie is not registered because there is a limit to the cookie size, which is perfectly okay > document.cookie < "" Expected results: I think there should be a warning on the developer tools console, telling me I've gone over the cookie size limit, so that I can be wary of it as I develop applications.
Severity: normal → enhancement
Component: Untriaged → Networking: Cookies
Product: Firefox → Core
Whiteboard: [necko-backlog]
Assignee: nobody → amchung
Whiteboard: [necko-backlog] → [necko-next]
Whiteboard: [necko-next] → [necko-active]
Hi Ehsan, I have added a notify function triggering when cookie size is too big, and my modifications are below: 1. Added a notify function. 2. Added a test case for this notify function. 3. Added Waring message on web-console in js term. Please help me to confirm my patch, thanks!
Attachment #8798057 - Flags: review?(ehsan)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Modified some comments.
Attachment #8798057 - Attachment is obsolete: true
Attachment #8798057 - Flags: review?(ehsan)
Attachment #8798132 - Flags: review?(ehsan)
Comment on attachment 8798132 [details] [diff] [review] Added a Notify funtion when cookie oversize. Review of attachment 8798132 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the cookie bits. I have a few small comments on the cookie parts but they should all be easy to address without requesting another review from me. But this needs a devtools peer review. Once that's done, if as a result you ended up making substantial changes to the cookie part of the patch, I'd be happy to review it again. Thanks! ::: devtools/client/webconsole/jsterm.js @@ +247,5 @@ > init: function () { > + observerService.addObserver(function observer() { > + cookieErrorWarning = true; > + observerService.removeObserver(observer, "cookie-oversize"); > + }, "cookie-oversize", false); Please get review on this part from a peer of this code, since I'm not very familiar with it. That being said, it seems like your code here only reports this warning once, which seems strange. Also, it seems like you're not using the URI argument that's delivered as part of that notification. If you don't need that, please remove it. @@ +391,5 @@ > } > + > + var msg; > + if (cookieErrorWarning) { > + let errorMsg = "Cookie size is too big (>kMaxBytesPerCookie)\n"; This is almost certainly not OK, because the error message needs to be localizable... ::: netwerk/cookie/nsCookieService.cpp @@ +2082,5 @@ > +nsCookieService::NotifyOversize(nsIURI *aHostURI) > +{ > + nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); > + if (os) { > + os->NotifyObservers(aHostURI, "cookie-oversize", nullptr); See my comment about this URI argument. One thing to think about is how the devtools UI should show what tab this message belongs to. I'm not familiar with how that's done on the devtools side, but if we need to, we can capture the inner window ID here and send it along the notification for the devtools UI to use it to tie the message to a specific window. ::: netwerk/cookie/test/unit/test_bug1293067.js @@ +10,5 @@ > + > +function run_test() { > + var tests = []; > + ob.addObserver(function observer() { > + ok(true, "cookie size is oversize"); Nothing is actually checking that the notification has been dispatched here. Usually for cases like this, you define a variable such as wasOversideNotificationDispatched, and set it to true here, and check its value after test_cookie_oversize() has yielded. @@ +27,5 @@ > + const BASE_URI = Services.io.newURI("http://example.org/", null, null); > + > + yield setCookie("foo", Array(maxBytePerCookie + 1).join('x'), null, null, null, BASE_URI); > + // Confirm the "foo" cookie doesn't set in cookie DB. > + do_check_eq(0, cm.countCookiesFromHost(BASE_URI.host)); Can you please also test the case where the size of the cookie is equal to maxBytePerCookie? @@ +48,5 @@ > + } else { > + s += ' (session)'; > + } > + s += ' for ' + url.spec; > + do_print(s); This is only useful for debugging. Please remove the debug output.
Attachment #8798132 - Flags: review?(ehsan) → review+
Comment on attachment 8798132 [details] [diff] [review] Added a Notify funtion when cookie oversize. Review of attachment 8798132 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/webconsole/jsterm.js @@ +384,4 @@ > return; > } > > if (this.hud.NEW_CONSOLE_OUTPUT_ENABLED) { Current webconsole is during the transition to new webconsole. Here is the entry point which we set up a flag to switch to new version (only enable in nightly by default). It's supposed to pass a cookieErrorWarning here as well. However, I'm not the person who is familiar with this part to make decision where is the right place to wrap cookieErrorWarning. I think linclark is responsible for the webconsole, so please set her as the second reviewer would be helpful. :)
Hi Ehsan, I have modified code from your suggestions as below: 1. Modified the argument from uri to the inner window ID and send it along the notification for the devtools UI to use it. 2. Modified test cases. Please help me to review my patch, thanks!
Attachment #8798132 - Attachment is obsolete: true
Attachment #8812637 - Flags: review?(ehsan)
Hi Lin, I have a questions for NEW_CONSOLE_OUTPUT_ENABLED. If I would like to add a error message when setting cookie and length of cookie is oversize, how do I send the error msg to new console? The patch can work when disabling NEW_CONSOLE_OUTPUT_ENABLED, please give me your suggestions. Thanks!
Attachment #8812725 - Flags: feedback?(lclark)
Comment on attachment 8812725 [details] [diff] [review] Added a error msg when setting cookie and length of cookie is oversize. Review of attachment 8812725 [details] [diff] [review]: ----------------------------------------------------------------- Hi Amy, I'm not working on the console anymore, so I'm going to forward this feedback request to the two folks who are.
Attachment #8812725 - Flags: feedback?(lclark)
Attachment #8812725 - Flags: feedback?(chevobbe.nicolas)
Attachment #8812725 - Flags: feedback?(bgrinstead)
Comment on attachment 8812637 [details] [diff] [review] Added a Notify funtion when cookie oversize. Review of attachment 8812637 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is really close, but I'd like to look over the test again after you've addressed the comments below. ::: netwerk/cookie/nsCookieService.cpp @@ +2096,5 @@ > + if (os) { > + nsresult rv; > + nsCOMPtr<nsISupportsPRUint64> InnerWindowId = > + (do_CreateInstance(NS_SUPPORTS_PRUINT64_CONTRACTID, &rv)); > + if (NS_FAILED(rv)) { Nit: please wrap in an NS_WARN_IF() so that we get a warning dumped out if this fails. @@ +3320,5 @@ > + if (!loadInfo) { > + return 0; > + } > + > + windowId = loadInfo->GetInnerWindowID(); You can declare and initialize this in one shot here, so that you don't need to declare windowId above. ::: netwerk/cookie/nsCookieService.h @@ +9,5 @@ > #include "nsICookieService.h" > #include "nsICookieManager.h" > #include "nsICookieManager2.h" > #include "nsIObserver.h" > +#include "nsISupportsPrimitives.h" Please move this to the .cpp file where it's actually needed. ::: netwerk/cookie/test/unit/test_bug1293067.js @@ +26,5 @@ > +// Verify that cookies that set to oversize and confirm it doesn't set in DB. > +function* test_cookie_oversize() { > + const BASE_URI = Services.io.newURI("http://example.org/", null, null); > + > + yield setCookie("foo", Array(maxBytePerCookie + 1).join('x'), null, null, null, BASE_URI); Like I asked before, can you please also add a test for when the size is exactly maxBytePerCookie bytes? @@ +32,5 @@ > + do_check_eq(0, cm.countCookiesFromHost(BASE_URI.host)); > + do_check_true(wasOversideNotificationDispatched,"Cookie oversize"); > + > + // the len of cookie name + the len of cookie value < maxBytePerCookie > + yield setCookie("foo2", Array(maxBytePerCookie - 4).join('x'), null, null, null, BASE_URI); Before doing this, you want to reset wasOversideNotificationDispatched to false, and then check its value afterwards to make sure that the notification wasn't dispatched before.
Attachment #8812637 - Flags: review?(ehsan)
Comment on attachment 8812725 [details] [diff] [review] Added a error msg when setting cookie and length of cookie is oversize. Review of attachment 8812725 [details] [diff] [review]: ----------------------------------------------------------------- Please use nsContentUtils::ReportToConsole instead of an observer service: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp?#3602. This will fully support both console frontends and can move the localized string into the platform where the warning is being emitted. For another example of this type of warning being logged to the console, see the mixed content warning displayed on this page: https://people-mozilla.org/~tvyas/mixeddisplay.html. That logs the LoadingMixedDisplayContent2 string in the LogMixedContentMessage function here: https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#226
Attachment #8812725 - Flags: feedback?(chevobbe.nicolas)
Attachment #8812725 - Flags: feedback?(bgrinstead)
Attached patch bug-1293067v5.patch (obsolete) — — Splinter Review
Hi Nicolas, In my patch, I have added nsContentUtils::ReportToConsole, but I found the warning msg which be showed on ToolBox. Do I use wrong setting on nsContentUtils::ReportToConsole? Please give me your suggestions, thanks a lot!
Attachment #8820219 - Flags: feedback?(chevobbe.nicolas)
Hi Amy, I'll try to have a look at this today
Comment on attachment 8820219 [details] [diff] [review] bug-1293067v5.patch Review of attachment 8820219 [details] [diff] [review]: ----------------------------------------------------------------- I think you'll need a necko peer to review this patch, not a devtools peer. Since you're editing code in netwerk/cookie, I would say one of these people would be appropriate: https://wiki.mozilla.org/Modules/All#Cookies_and_Permissions ::: netwerk/cookie/nsCookieService.cpp @@ +3304,5 @@ > + doc = do_QueryInterface(domDoc); > + } > + } else { > + return; > + } Maybe the logic would be better like this: if (!aChannel) { return; } nsCOMPtr<nsIDocument> doc; nsCOMPtr<nsILoadInfo> aLoadInfo; aChannel->GetLoadInfo(getter_AddRefs(aLoadInfo)); if (!aLoadInfo) { return; } nsCOMPtr<nsIDOMDocument> domDoc; aLoadInfo->GetLoadingDocument(getter_AddRefs(domDoc)); if (domDoc) { doc = do_QueryInterface(domDoc); } nsContentUtils::ReportToConsole(...); @@ +3306,5 @@ > + } else { > + return; > + } > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > + NS_LITERAL_CSTRING("Cookie Oversize"), I think the string should localized, but I don't know enough about our l10n policies. ::: netwerk/cookie/test/unit/test_bug1293067.js @@ +20,5 @@ > + yield test_cookie_oversize(); > + }); > + > + run_next_test(); > +} The indentation should be 2 spaces. @@ +42,5 @@ > + let cookie = name + "=" + value; > + var s = 'setting cookie ' + value; > + if (domain) { > + cookie += "; Domain=" + domain; > + s += ' (d=' + domain + ')'; You can use a template string here (and pretty much most places in this file). cookie += `; Domain=${domain}` @@ +62,5 @@ > + // Windows XP has low precision timestamps that cause our cookie eviction > + // algorithm to produce different results from other platforms. We work around > + // this by ensuring that there's a clear gap between each cookie update. > + do_timeout(10, resolve); > + }) nit: missing semi I wonder why eslint isn't enabled for necko. ::: netwerk/locales/en-US/necko.properties @@ +43,5 @@ > # LOCALIZATION NOTE (APIDeprecationWarning): > # %1$S is the deprecated API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > +CookieOverSize=The cookie is oversize (>4k). nit: is oversized
Hi Amy, sorry for the delay on this. I dug the code a little and it seems to me that ReportToConsole does target the BrowserConsole, and not the one from the devtools (I can be wrong though). I tracked what is done for CSS warnings, which led me to this http://searchfox.org/mozilla-central/source/layout/style/ErrorReporter.cpp#206-218 . Hope it helps, thank you for working on this !
Attachment #8820219 - Flags: feedback?(chevobbe.nicolas) → feedback?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13) > Hi Amy, sorry for the delay on this. > I dug the code a little and it seems to me that ReportToConsole does target > the BrowserConsole, and not the one from the devtools (I can be wrong > though). > > I tracked what is done for CSS warnings, which led me to this > http://searchfox.org/mozilla-central/source/layout/style/ErrorReporter. > cpp#206-218 . > > Hope it helps, thank you for working on this ! Thanks your suggestions!
Hi Ehsan, I have a questions for this bug: I found the code flow is nsContentDLF::CreateDocument=>nsHTMLDocument::StartDocumentLoad=>nsHTMLDocument::SetCookie when using "document.cooke" on webconsole. And nsIDocument can't get from nsIChannel on nsCookieService and CookieServiceChild made the warning msg which be showed on ToolBox after calling nsContentUtils::ReportToConsole. In my view, maybe we would create a function on nsICookieService for confirming cookie oversize and add nsContentUtils::ReportToConsole on nsHTMLDocument::SetCookie(the argument of nsIDocument set to "this"). Would you give me your suggestion? Thanks!
Flags: needinfo?(ehsan)
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #12) > Comment on attachment 8820219 [details] [diff] [review] > bug-1293067v5.patch > > Review of attachment 8820219 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think you'll need a necko peer to review this patch, not a devtools peer. > > Since you're editing code in netwerk/cookie, I would say one of these people > would be appropriate: > https://wiki.mozilla.org/Modules/All#Cookies_and_Permissions > > ::: netwerk/cookie/nsCookieService.cpp > @@ +3304,5 @@ > > + doc = do_QueryInterface(domDoc); > > + } > > + } else { > > + return; > > + } > > Maybe the logic would be better like this: > > if (!aChannel) { > return; > } > nsCOMPtr<nsIDocument> doc; > nsCOMPtr<nsILoadInfo> aLoadInfo; > aChannel->GetLoadInfo(getter_AddRefs(aLoadInfo)); > if (!aLoadInfo) { > return; > } > nsCOMPtr<nsIDOMDocument> domDoc; > aLoadInfo->GetLoadingDocument(getter_AddRefs(domDoc)); > if (domDoc) { > doc = do_QueryInterface(domDoc); > } > > nsContentUtils::ReportToConsole(...); > > @@ +3306,5 @@ > > + } else { > > + return; > > + } > > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > > + NS_LITERAL_CSTRING("Cookie Oversize"), > > I think the string should localized, but I don't know enough about our l10n > policies. > > ::: netwerk/cookie/test/unit/test_bug1293067.js > @@ +20,5 @@ > > + yield test_cookie_oversize(); > > + }); > > + > > + run_next_test(); > > +} > > The indentation should be 2 spaces. > > @@ +42,5 @@ > > + let cookie = name + "=" + value; > > + var s = 'setting cookie ' + value; > > + if (domain) { > > + cookie += "; Domain=" + domain; > > + s += ' (d=' + domain + ')'; > > You can use a template string here (and pretty much most places in this > file). > cookie += `; Domain=${domain}` > > @@ +62,5 @@ > > + // Windows XP has low precision timestamps that cause our cookie eviction > > + // algorithm to produce different results from other platforms. We work around > > + // this by ensuring that there's a clear gap between each cookie update. > > + do_timeout(10, resolve); > > + }) > > nit: missing semi > > I wonder why eslint isn't enabled for necko. > > ::: netwerk/locales/en-US/necko.properties > @@ +43,5 @@ > > # LOCALIZATION NOTE (APIDeprecationWarning): > > # %1$S is the deprecated API; %2$S is the API function that should be used. > > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > > > +CookieOverSize=The cookie is oversize (>4k). > > nit: is oversized Thanks for your help!
(In reply to Amy Chung [:Amy] from comment #15) > Hi Ehsan, > I have a questions for this bug: > I found the code flow is > nsContentDLF::CreateDocument=>nsHTMLDocument:: > StartDocumentLoad=>nsHTMLDocument::SetCookie when using "document.cooke" on > webconsole. > And nsIDocument can't get from nsIChannel on nsCookieService and > CookieServiceChild made the warning msg which be showed on ToolBox after > calling nsContentUtils::ReportToConsole. > In my view, maybe we would create a function on nsICookieService for > confirming cookie oversize and add nsContentUtils::ReportToConsole on > nsHTMLDocument::SetCookie(the argument of nsIDocument set to "this"). > Would you give me your suggestion? nsHTMLDocument::CreateDummyChannelForCookies() is responsible for creating the dummy channel object that SetCookie() uses in this case. I think if you set a load info on this channel object that has a pointer to the document, the error reporting code would work equally well for both the normal case and the case of setting document.cookie like you describe above.
Flags: needinfo?(ehsan)
Hi Ehsan, Thanks for your suggestions, and I finished to implement part. The modifications as below: 1. Created a new nsILoadInfo and set nsHTMLDocument to nsILoadInfo. 2. Created a function GetCookieOverSized on ipdl for confirming whether cookie is oversized. Would you please review my patch?
Attachment #8820219 - Attachment is obsolete: true
Attachment #8820219 - Flags: feedback?
Attachment #8823810 - Flags: feedback?(ehsan)
Comment on attachment 8823810 [details] [diff] [review] Part1: implement: Added a ReportToConsole on CookieService. Review of attachment 8823810 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand the need to do IPC in this patch... It seems that it can be simplified a lot by doing everything in the child process as I mentioned below. If the IPC path was needed because of a reason I'm missing, please let me know. ::: dom/html/nsHTMLDocument.cpp @@ +1339,5 @@ > if (!channel) { > return; > } > + } else { > + nsCOMPtr<nsILoadInfo> loadInfo = Please add a comment saying why you're setting up a load info. Suggestion: // Set up a dummy loadinfo object for the sole purpose of being able // to retrieve the document later on through GetLoadingDocument() for // error reporting purposes. @@ +1342,5 @@ > + } else { > + nsCOMPtr<nsILoadInfo> loadInfo = > + new LoadInfo(nsContentUtils::GetSystemPrincipal(), > + nullptr,this, > + nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, Hmm, this doesn't matter in practice since nothing will be loaded from this channel, but this looks like an odd default choice. Why not use something more conservative, like SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS? ::: dom/locales/en-US/chrome/dom/dom.properties @@ +1,5 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +CookieOverSize=Cookie Over Size This string is unused. ::: netwerk/cookie/CookieServiceChild.cpp @@ +16,4 @@ > #include "nsIPrefService.h" > #include "nsIPrefBranch.h" > #include "nsServiceManagerUtils.h" > +//delete ??? @@ +145,5 @@ > > return NS_OK; > } > > +void ReportCookieOverSized(nsILoadInfo *loadInfo) Nit: make this static please. @@ +157,5 @@ > + doc = do_QueryInterface(domDoc); > + > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > + NS_LITERAL_CSTRING("Cookie Manager"), > + doc, You should null check doc before using it here. @@ +205,5 @@ > // Synchronously call the parent. > SendSetCookieString(uriParams, !!isForeign, cookieString, serverTime, > aFromHttp, attrs); > + bool ConfirmCookieOverSized = false; > + SendGetCookieOverSized(cookieString,&ConfirmCookieOverSized); I'm not sure if I understand why you're doing IPC here. Are you trying to keep the parent process in charge of checking the cookie size? I think that's pointless, since if the child can't be trusted to check the size, it can also not be trusted to properly report it as oversize. It seems to me that this can be easily checked right here in the child process. ::: netwerk/cookie/nsCookieService.cpp @@ +3301,5 @@ > +} > + > + > +void > +ReportCookieOversizeToConsole(nsIChannel *aChannel) This is almost duplicated code. Please avoid duplication like this. ::: netwerk/locales/en-US/necko.properties @@ +43,5 @@ > # LOCALIZATION NOTE (APIDeprecationWarning): > # %1$S is the deprecated API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > +CookieOverSized=The cookie is oversized (>4k). I think this message can be improved. How about something like this? Warning: dropping cookie named "foo" because it was larger than the biggest maximum supported cookie (4KB). (This means that you would need to include the cookie name in the message.)
Attachment #8823810 - Flags: feedback?(ehsan) → feedback-
Hi Ehsan, Thanks your suggestions, I have modified my patch as below: 1. Added comment on dom/html/nsHTMLDocument.cpp. 2. Modified the argument nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL to nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS when creating a dummy LoadInfo. 3. Removed "CookieOverSize" from dom.properties. 4. Set ReportCookieOverSized to static. 5. Confirmed doc before calling nsContentUtils::ReportToConsole. 6. Removed ipc. the reason of using ipc: I would like to use "nsCookieService::ParseAttributes(aCookieHeader, cookieAttributes)" to get cookie name and value. Because I thought rewrite a same parsing string function in CookieServiceChild is too complicated. 7. Wrote a simply function to get len of cookie name and value. 8. Removed ReportCookieOversizeToConsole in nsCookieService.cpp. 9. Modified warning msg in necko.properties. Please give me your suggestions and review my patch. Thanks!
Attachment #8823810 - Attachment is obsolete: true
Attachment #8824033 - Flags: feedback?(ehsan)
(In reply to Amy Chung [:Amy] from comment #20) > the reason of using ipc: I would like to use > "nsCookieService::ParseAttributes(aCookieHeader, cookieAttributes)" to get > cookie name and value. > Because I thought rewrite a same parsing string function in > CookieServiceChild is too complicated. Oh, I see! nsCookieService::ParseAttributes() is already a static function that can be called without the rest of nsCookieService being available. It's protected by default, but you can make CookieServiceChild also a friend (similar to <http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/netwerk/cookie/nsCookieService.h#370>) and call this method directly.
Comment on attachment 8824033 [details] [diff] [review] Part1: implement: Added a ReportToConsole on CookieService. Review of attachment 8824033 [details] [diff] [review]: ----------------------------------------------------------------- This looks generally good besides the code duplications, please see my comments below. I haven't done a full review yet because a substantial part of the patch will probably change. ::: netwerk/cookie/CookieServiceChild.cpp @@ +145,5 @@ > > return NS_OK; > } > > +static void ReportCookieOverSized(nsILoadInfo *aLoadInfo, nsDependentCString aCookieName) This function is still duplicated. You can make this one also a static member of nsCookieService and call it from both places... @@ +210,5 @@ > // Synchronously call the parent. > SendSetCookieString(uriParams, !!isForeign, cookieString, serverTime, > aFromHttp, attrs); > + > + int32_t nameAndValueLen = 0; As I said in comment 21, you don't need to duplicate this code here. ::: netwerk/locales/en-US/necko.properties @@ +43,5 @@ > # LOCALIZATION NOTE (APIDeprecationWarning): > # %1$S is the deprecated API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > +CookieOverSized=Warning: dropping cookie named "%1$S" because it was larger than the biggest maximum supported cookie (4KB). The rest of the strings here are using these fancy quotes like “%1$S”. Also, it may be a good idea to add a LOCALIZATION NOTE explaining that "cookie" here means an HTTP cookie, not the edible kind, to make our localizers' lives easier.
Attachment #8824033 - Flags: feedback?(ehsan) → feedback+
Hi Ehsan, Thanks for your suggestions, I have modified my patch as below: 1. Added a static function nsCookieService::ReportCookieOverSized(). 2. Removed a static function ReportCookieOverSized() in CookieServiceChild.cpp. 3. Used "HTTP cookie" instead to "cookie" on warning message. Please help me to review my patch, thanks!
Attachment #8824033 - Attachment is obsolete: true
Attachment #8825010 - Flags: review?(ehsan)
Hi Ehsan, I have added test cases and testing items as below: 1. Confirmed the warning msg when set oversized cookie. 2. Confirmed the oversized cookie doesn't exist. 3. Confirmed the normal output when set normal cookie. 4. Confirmed the normal cookie which exists. Please help me to review my patch, thanks!
Attachment #8825011 - Flags: review?(ehsan)
Hi Amy, I'm at a work week in Taipei this week, so I may not get to review this before next week. I'll review as soon as I can...
Comment on attachment 8825010 [details] [diff] [review] Part1: implement: Added a ReportToConsole on CookieService. Review of attachment 8825010 [details] [diff] [review]: ----------------------------------------------------------------- This patch lacks any handling for the non-e10s case... ::: netwerk/cookie/CookieServiceChild.cpp @@ +8,5 @@ > #include "mozilla/BasePrincipal.h" > #include "mozilla/ipc/URIUtils.h" > #include "mozilla/net/NeckoChild.h" > +#include "nsContentUtils.h" > +#include "nsIScriptError.h" You don't need these #includes. @@ +16,5 @@ > #include "nsIPrefService.h" > #include "nsIPrefBranch.h" > #include "nsServiceManagerUtils.h" > +#include "nsIDocument.h" > +#include "nsIDOMDocument.h" You don't need these #includes. @@ +193,5 @@ > + int32_t valueEndLocation = cookieString.Find(";"); > + // Set cookie to "name=value; domain=...." > + if (nameEndLocation > 0 > + && valueEndLocation > 0 > + && nameEndLocation < valueEndLocation) { Nit: please put && and || and the end of lines, not the beginning, like the rest of our code. For example: if (nameEndLocation > 0 && valueEndLocation > 0 && ...) @@ +202,5 @@ > + > + // Set cookie to "value; domain=...." or "value; session" > + } else if ((nameEndLocation > valueEndLocation > + || nameEndLocation < 0) > + && valueEndLocation > 0) { Here too. @@ +210,5 @@ > + } else { > + nameAndValueLen = cookieString.Length(); > + } > + > + cookieString.Cut(nameEndLocation,cookieString.Length()); Nit: space after comma ::: netwerk/cookie/nsCookieService.cpp @@ +3348,5 @@ > COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader, "cookie too big (> 4kb)"); > + if (!aChannel) { > + return newCookie; > + } > + ReportCookieOverSized(aChannel, cookieAttributes.name); I think it would be cleaner if you did: if (aChannel) { ReportCookieOverSized(...); } return newCookie; ::: netwerk/locales/en-US/necko.properties @@ +43,5 @@ > # LOCALIZATION NOTE (APIDeprecationWarning): > # %1$S is the deprecated API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > +CookieOverSized=Warning: dropping HTTP cookie named "%1$S" because it was larger than the biggest maximum supported HTTP cookie (4KB). Please add the LOCALIZATION NOTE I requested before.
Attachment #8825010 - Flags: review?(ehsan) → review-
Comment on attachment 8825011 [details] [diff] [review] Part2: test cases. Review of attachment 8825011 [details] [diff] [review]: ----------------------------------------------------------------- The test itself looks good but given the part 1 patch, it should fail when you run it in non-e10s mode. Can you please verify that the test fails in this scenario, and retest in both modes after fixing part 1? Thanks!
Attachment #8825011 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #26) > ::: netwerk/locales/en-US/necko.properties > @@ +43,5 @@ > > # LOCALIZATION NOTE (APIDeprecationWarning): > > # %1$S is the deprecated API; %2$S is the API function that should be used. > > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > > > +CookieOverSized=Warning: dropping HTTP cookie named "%1$S" because it was larger than the biggest maximum supported HTTP cookie (4KB). > > Please add the LOCALIZATION NOTE I requested before. Hi Ehsan, Thanks for you suggestions, I have a question about LOCALIZATION : In my view, I added the CookieOverSized in necko.properties that is LOCALIZATION. Should I update LOCALIZATION on webconsole.properties? Thanks!
Flags: needinfo?(ehsan)
Sorry I think I may have confused you... Localization notes are specially formatted markers that serve as comments directed to the localizers. We have a lot of examples, see the results from <http://searchfox.org/mozilla-central/search?q=LOCALIZATION%20NOTE&path=>. This is the way we signal some information about the localized string that may not be obvious. For example, at the end of comment 22 I suggested adding a note stating that "cookie" here means "HTTP cookie" not cookies that we eat. :-)
Flags: needinfo?(ehsan)
Hi Ehsan, I have modified the patch from your suggestions, 1. Removed excess including header. 2. Modified Nit. 3. Modified if condition. 4. Added LOCALIZATION NOTE And I already confirmed the test cases can pass on non-e10s mode. Would you help me to review my patch? Thanks!
Attachment #8825010 - Attachment is obsolete: true
Attachment #8828738 - Flags: review?(ehsan)
Comment on attachment 8828738 [details] [diff] [review] Part1: implementation: Added a ReportToConsole on CookieService. Review of attachment 8828738 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks great!
Attachment #8828738 - Flags: review?(ehsan) → review+
This patch caused some error on try server, I will found the root cause and fix it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=469683a8e7f45002e7ad26365b9afd85682efe62
It seems all these errors were coming from same assertion failure. Are you able to reproduce it locally with debug build? Assertion failure: !aLoadingContext || !aLoadingPrincipal || aLoadingContext->NodePrincipal() == aLoadingPrincipal, at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/netwerk/base/LoadInfo.cpp:94
Whiteboard: [necko-active] → [necko-backlog]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Assignee: amchung → amarchesini
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8ed41aa3ed4f Implement a cookie limit warning, r=mayhemer,flod https://hg.mozilla.org/integration/autoland/rev/5df9d39b0b98 Implement a cookie limit warning - tests, r=mayhemer

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&test_paths=netwerk%2Fcookie%2Ftest%2Fbrowse&revision=5df9d39b0b98eb17eecad01752b736a28c316cc6&selectedJob=294653265

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=294653265&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/0577d24b6ca5be503767d9e82536d431bcabe575

[task 2020-03-25T11:24:09.885Z] 11:24:09 INFO - TEST-START | netwerk/cookie/test/browser/browser_oversize.js
[task 2020-03-25T11:24:09.941Z] 11:24:09 INFO - GECKO(10988) | [Child 13007: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x7f770a337800 == 0 [pid = 13007] [id = {2890603c-14db-49f8-8aaf-0cbb852ec58a}] [url = http://example.com/browser/netwerk/cookie/test/browser/file_empty.html]
[task 2020-03-25T11:24:10.001Z] 11:24:10 INFO - GECKO(10988) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpvrTcbd.mozrunner/runtests_leaks_tab_pid13034.log
[task 2020-03-25T11:24:10.038Z] 11:24:10 INFO - GECKO(10988) | [Child 13007: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (0x7f7725893520) [pid = 13007] [serial = 1] [outer = (nil)] [url = http://example.com/browser/netwerk/cookie/test/browser/file_empty.html]
[task 2020-03-25T11:24:10.038Z] 11:24:10 INFO - GECKO(10988) | [Child 13007: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 1 (0x7f770a66c000) [pid = 13007] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2020-03-25T11:24:10.038Z] 11:24:10 INFO - GECKO(10988) | [Child 13007: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 0 (0x7f770afcb800) [pid = 13007] [serial = 3] [outer = (nil)] [url = http://example.com/browser/netwerk/cookie/test/browser/file_empty.html]
[task 2020-03-25T11:24:10.079Z] 11:24:10 INFO - GECKO(10988) | [Child 13007, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp, line 3352
[task 2020-03-25T11:24:10.086Z] 11:24:10 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: '!mName', file /builds/worker/checkouts/gecko/editor/libeditor/EditAggregateTransaction.cpp, line 91
[task 2020-03-25T11:24:10.086Z] 11:24:10 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: EditAggregationTransaction::GetName() failed: 'NS_SUCCEEDED(rv)', file /builds/worker/checkouts/gecko/editor/libeditor/PlaceholderTransaction.cpp, line 217
[task 2020-03-25T11:24:10.086Z] 11:24:10 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: nsIAbsorbingTransaction::GetTxnName() failed, but ignored: 'NS_SUCCEEDED(rvIgnored)', file /builds/worker/checkouts/gecko/editor/libeditor/PlaceholderTransaction.cpp, line 188
[task 2020-03-25T11:24:10.127Z] 11:24:10 INFO - GECKO(10988) | [Child 13034, Main Thread] WARNING: No CID found when attempting to map contract ID: file /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp, line 721
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | nsStringStats
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | => mAllocCount: 8743
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | => mReallocCount: 0
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | => mFreeCount: 8743
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | => mShareCount: 5330
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | => mAdoptCount: 325
[task 2020-03-25T11:24:10.143Z] 11:24:10 INFO - GECKO(10988) | => mAdoptFreeCount: 329
[task 2020-03-25T11:24:10.144Z] 11:24:10 INFO - GECKO(10988) | => Process ID: 13007, Thread ID: 140149731878784
[task 2020-03-25T11:24:10.180Z] 11:24:10 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: NS_ENSURE_TRUE(GetWrapper()) failed: file /builds/worker/checkouts/gecko/dom/ipc/JSWindowActor.cpp, line 66
[task 2020-03-25T11:24:10.369Z] 11:24:10 INFO - GECKO(10988) | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2020-03-25T11:24:10.426Z] 11:24:10 INFO - GECKO(10988) | [Child 12409: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x7ff52a535800 == 0 [pid = 12409] [id = {7ab6be2e-2768-4caa-9e97-517624d6827d}] [url = about:blank]
[task 2020-03-25T11:24:10.567Z] 11:24:10 INFO - GECKO(10988) | [Child 13034, Main Thread] WARNING: could not set real-time limit at process startup: file /builds/worker/checkouts/gecko/dom/ipc/ContentChild.cpp, line 1701
[task 2020-03-25T11:24:10.588Z] 11:24:10 INFO - GECKO(10988) | [Child 13034: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7fd923638000 == 1 [pid = 13034] [id = {ef16ece2-c888-49a5-a91a-29df3352f343}]
[task 2020-03-25T11:24:10.603Z] 11:24:10 INFO - GECKO(10988) | [Child 13034: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x7fd93eb93520) [pid = 13034] [serial = 1] [outer = (nil)]
[task 2020-03-25T11:24:10.604Z] 11:24:10 INFO - GECKO(10988) | [Child 13034, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/base/nsPresContext.cpp, line 839
[task 2020-03-25T11:24:10.605Z] 11:24:10 INFO - GECKO(10988) | [Child 13034: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x7fd929847800) [pid = 13034] [serial = 2] [outer = 0x7fd93eb93520]
[task 2020-03-25T11:24:10.745Z] 11:24:10 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: NS_ENSURE_TRUE(mIsPending) failed: file /builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp, line 9619
[task 2020-03-25T11:24:10.746Z] 11:24:10 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: NS_ENSURE_TRUE(mSuspendCount > 0) failed: file /builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpChannel.cpp, line 9660
[task 2020-03-25T11:24:10.762Z] 11:24:10 INFO - GECKO(10988) | [Child 13034: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (0x7fd923974400) [pid = 13034] [serial = 3] [outer = 0x7fd93eb93520]
[task 2020-03-25T11:24:11.474Z] 11:24:11 INFO - GECKO(10988) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpvrTcbd.mozrunner/runtests_leaks_tab_pid13063.log
[task 2020-03-25T11:24:11.575Z] 11:24:11 INFO - GECKO(10988) | [Child 13063, Main Thread] WARNING: No CID found when attempting to map contract ID: file /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp, line 721
[task 2020-03-25T11:24:11.718Z] 11:24:11 INFO - GECKO(10988) | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2020-03-25T11:24:11.819Z] 11:24:11 INFO - GECKO(10988) | [Child 13063, Main Thread] WARNING: could not set real-time limit at process startup: file /builds/worker/checkouts/gecko/dom/ipc/ContentChild.cpp, line 1701
[task 2020-03-25T11:24:20.097Z] 11:24:20 INFO - GECKO(10988) | [Child 12409: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 1 (0x7ff545a93520) [pid = 12409] [serial = 1] [outer = (nil)] [url = about:blank]
[task 2020-03-25T11:24:22.903Z] 11:24:22 INFO - GECKO(10988) | [Child 13034: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (0x7fd929847800) [pid = 13034] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2020-03-25T11:24:24.131Z] 11:24:24 INFO - GECKO(10988) | [Child 12409: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 0 (0x7ff52a837c00) [pid = 12409] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2020-03-25T11:25:40.130Z] 11:25:40 INFO - TEST-INFO | started process screentopng
[task 2020-03-25T11:25:40.712Z] 11:25:40 INFO - TEST-INFO | screentopng: exit 0
[task 2020-03-25T11:25:40.712Z] 11:25:40 INFO - Buffered messages logged at 11:24:09
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Entering test bound
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Buffered messages logged at 11:24:10
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Console Listener: [JavaScript Warning: "Cookie “a” will be soon rejected because it has the “sameSite” attribute set to “none” or an invalid value, without the “secure” attribute. To know more about the “sameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Cookies" {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Console message: [JavaScript Warning: "Cookie “a” will be soon rejected because it has the “sameSite” attribute set to “none” or an invalid value, without the “secure” attribute. To know more about the “sameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Cookies" {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Console Listener: [JavaScript Warning: "Cookie “a” is invalid because its size is too big. Max size is 4096 B." {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Console message: [JavaScript Warning: "Cookie “a” is invalid because its size is too big. Max size is 4096 B." {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.713Z] 11:25:40 INFO - Console Listener: [JavaScript Warning: "Cookie “b” will be soon rejected because it has the “sameSite” attribute set to “none” or an invalid value, without the “secure” attribute. To know more about the “sameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Cookies" {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.714Z] 11:25:40 INFO - Console message: [JavaScript Warning: "Cookie “b” will be soon rejected because it has the “sameSite” attribute set to “none” or an invalid value, without the “secure” attribute. To know more about the “sameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Cookies" {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.715Z] 11:25:40 INFO - Console Listener: [JavaScript Warning: "Cookie “b” is invalid because its path size is too big. Max size is 1024 B." {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.715Z] 11:25:40 INFO - Console message: [JavaScript Warning: "Cookie “b” is invalid because its path size is too big. Max size is 1024 B." {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.717Z] 11:25:40 INFO - Console Listener: [JavaScript Error: "The character encoding of the plain text document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the file needs to be declared in the transfer protocol or file needs to use a byte order mark as an encoding signature." {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.718Z] 11:25:40 INFO - Console message: [JavaScript Error: "The character encoding of the plain text document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the file needs to be declared in the transfer protocol or file needs to use a byte order mark as an encoding signature." {file: "http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs" line: 0}]
[task 2020-03-25T11:25:40.718Z] 11:25:40 INFO - Buffered messages finished
[task 2020-03-25T11:25:40.719Z] 11:25:40 INFO - TEST-UNEXPECTED-FAIL | netwerk/cookie/test/browser/browser_oversize.js | Test timed out -
[task 2020-03-25T11:25:40.719Z] 11:25:40 INFO - GECKO(10988) | MEMORY STAT | vsize 3028MB | residentFast 339MB | heapAllocated 104MB
[task 2020-03-25T11:25:40.719Z] 11:25:40 INFO - TEST-OK | netwerk/cookie/test/browser/browser_oversize.js | took 90252ms
[task 2020-03-25T11:25:40.720Z] 11:25:40 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-03-25T11:25:40.722Z] 11:25:40 INFO - TEST-UNEXPECTED-FAIL | netwerk/cookie/test/browser/browser_oversize.js | Found a tab after previous test timed out: http://example.com/browser/netwerk/cookie/test/browser/oversize.sjs -
[task 2020-03-25T11:25:40.722Z] 11:25:40 INFO - GECKO(10988) | [Child 13063: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7fdac6010800 == 1 [pid = 13063] [id = {414cd5ad-4b74-4b99-9aa5-2f57f080061f}]
[task 2020-03-25T11:25:40.722Z] 11:25:40 INFO - GECKO(10988) | [Child 13063: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x7fdae1293520) [pid = 13063] [serial = 1] [outer = (nil)]
[task 2020-03-25T11:25:40.723Z] 11:25:40 INFO - GECKO(10988) | [Child 13063, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/base/nsPresContext.cpp, line 839
[task 2020-03-25T11:25:40.723Z] 11:25:40 INFO - GECKO(10988) | [Child 13063: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x7fdac6994800) [pid = 13063] [serial = 2] [outer = 0x7fdae1293520]
[task 2020-03-25T11:25:40.723Z] 11:25:40 INFO - checking window state
[task 2020-03-25T11:25:40.724Z] 11:25:40 INFO - GECKO(10988) | [Parent 10988, Main Thread] WARNING: NS_ENSURE_TRUE(GetWrapper()) failed: file /builds/worker/checkouts/gecko/dom/ipc/JSWindowActor.cpp, line 66
[task 2020-03-25T11:25:40.727Z] 11:25:40 INFO - TEST-START | netwerk/cookie/test/browser/browser_sameSiteConsole.js

Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8a050f043234 Implement a cookie limit warning, r=mayhemer,flod

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox76: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78132a6cd59c Implement a cookie limit warning - tests, r=mayhemer
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: