Closed Bug 1475073 Opened Last year Closed Last year
Pass individual CORS errors as categories to web console error messages
The MDN team is creating CORS error pages that are meant to be linked to from Console: https://github.com/mdn/sprints/issues/64 To link pages to each individual error type the logs need to have individual categories (similar to JS errors). As the MDN content will be ready soon we should consider getting this uplifted into 62 if it lands in time and turns out low risk.
Christoph, follow up on our discussions, this would be a low effort/high impact project to improve messages. Who could help with adding log caregories?
The errors that need categories would be the following (from the linked Github issue) CORSDisabled CORSDidNotSucceed CORSOriginHeaderNotAdded CORSExternalRedirectNotAllowed CORSRequestNotHttp CORSMissingAllowOrigin CORSAllowOriginNotMatchingOrigin CORSNotSupportingCredentials CORSMethodNotFound CORSMissingAllowCredentials CORSPreflightDidNotSucceed CORSInvalidAllowMethod CORSInvalidAllowHeader CORSMissingAllowHeaderFromPreflight The easiest approach would be just re-using the localization keys above as categories. Implementation wise, the key would be handed through from `LogBlockedRequest` to `nsCORSListenerProxy::LogBlockedCORSRequest` (which right now always sends "CORS" as the category).
Just fore the refernece, I am assigning this bug to myself, similar to: https://bugzilla.mozilla.org/show_bug.cgi?id=1304645#c7
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P2
This patch passes all the CORS properties along as categories to the console. Whatever we decide to do within Bug 1304645, we should do the same here with this bug.
This looks good. Within the scope of this bug should also be to actually add the categories. Follow up work to link the bugs to MDN pages should happen in bug 1475391, as far as I understood :bgrins.
Same as in Bug 1304645 - renaming and only doing backend work within this bug - let's do all frontend/devtools related work in a follow up bug.
Summary: Add per-error-type categories for CORS console errors → Pass individual CORS errors as categories to web console error messages
Attachment #8992995 - Flags: review?(amarchesini)
Please note that all of those messages are prefixed with "CORS" anyway.
Comment on attachment 8992995 [details] [diff] [review] bug_1304645_change_category_for_cors_errors.patch Review of attachment 8992995 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsCORSListenerProxy.cpp @@ +85,5 @@ > return; > } > > nsAutoString msg(blockedMessage.get()); > + nsCString category(aProperty); nsDependentCString category(aProperty)
Attachment #8992995 - Flags: review?(amarchesini) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbeea69b10d Pass individual CORS errors as categories to web console error messages. r=baku
Backed out for failing mochitest and wpt Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4fbeea69b10d7487b74de22de78065d5b326dca5 Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=189130132&repo=mozilla-inbound&lineNumber=3535 https://treeherder.mozilla.org/logviewer.html#?job_id=189130548&repo=mozilla-inbound&lineNumber=7047 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3539fcf9699308a1a05d5cec055321bb1220b0d5
Ah, there is dom/base/test/test_warning_for_blocked_cross_site_request.html which checks the category in the console. Actually good that we have such tests. I updated that test to match the new behavior - should be fine now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=921326d8e8327c5c22dcd40a06963686c4e658a1
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbaf50e83c6 Pass individual CORS errors as categories to web console error messages. r=baku
Comment on attachment 8992995 [details] [diff] [review] bug_1304645_change_category_for_cors_errors.patch Approval Request Comment [Feature/Bug causing the regression]: No regression. MDN finished the documentation ahead of time, we just need the right categories in the console to start linking to them. [User impact if declined]: No major impact. CORS errors will not have links and remain less actionable [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: https://bugzilla.mozilla.org/attachment.cgi?id=8996257 [Is the change risky?]: Very low risk per :ckerschb [Why is the change risky/not risky?]: Covered by tests, only adds existing category texts to error logging. [String changes made/needed]: None.
Attachment #8992995 - Flags: approval-mozilla-beta?
Comment on attachment 8992995 [details] [diff] [review] bug_1304645_change_category_for_cors_errors.patch Looks useful for developers, low risk change adding categories to error messages. Let's uplift for beta 17 along with the patch in bug 1475391.
Attachment #8992995 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.