nsHttpChannel logs screwed up because LOG macro is forwarding to "PackagedAppService" module

RESOLVED FIXED in Firefox 50

Status

()

Core
Networking: HTTP
--
major
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

51 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
An hour of work crawling through a large log, locating the spot ended up finding out that nsHttpChannel doesn't log when nsHttp:5 is enabled.  Why?  Because LOG macro expands to PackagedAppService log module.

Not sure when this has happened, but I would more thing about a solution that would prevent this fault in the future.
(Assignee)

Comment 1

a year ago
Created attachment 8802899 [details] [diff] [review]
v1 (workaround ; rename LOG in PackagedAppService to LOG_PAS)

This is caused by some change made to the unified build ordering or simply by new files added that moved the boundary for unified file (whom size is limited).  PackagedAppService.cpp is now included before nsHttpChannel.cpp.

I wanted to rename LOG in protocol/http to LOGHTTP_<LEVEL> (my wish for a long time) but that is too much work.  This is a simle w/a that solves the problem.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8802899 - Flags: review?(valentin.gosu)
Comment on attachment 8802899 [details] [diff] [review]
v1 (workaround ; rename LOG in PackagedAppService to LOG_PAS)

Review of attachment 8802899 [details] [diff] [review]:
-----------------------------------------------------------------

That's an unexpected issue. Thanks for tracking it down.
I'm actually going to remove the packaged app service in the next few days, but we should definitely make sure this doesn't happen again.
Attachment #8802899 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 3

a year ago
No need for try, enough when builds and goes locally.
Keywords: checkin-needed
Whiteboard: [necko-active]

Comment 4

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5ebc082cb2
Rename LOG in PackagedAppService.cpp to avoid redefine of LOG directing to nsHttp log module. r=valentin
Keywords: checkin-needed
(Assignee)

Comment 5

a year ago
Comment on attachment 8802899 [details] [diff] [review]
v1 (workaround ; rename LOG in PackagedAppService to LOG_PAS)

Approval Request Comment
[Feature/regressing bug #]: not sure, hence requesting uplift

I'd like to make sure this is also on release, since that is also affected, in case we plan another dot build.

[User impact if declined]: none for the user, except some indirect:

There is a developer impact: logs for one of the most important parts of the platform, nsHttpChannel, are completely missing what makes analyzes of problems way harder or even completely impossible.

W/o this patch asking people on Release for http logs is impossible!

[Describe test coverage new/current, TreeHerder]: doesn't need any (doesn't have any..)
[Risks and why]: absolute zero
[String/UUID change made/needed]: none
Attachment #8802899 - Flags: approval-mozilla-release?
Attachment #8802899 - Flags: approval-mozilla-beta?
Attachment #8802899 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

a year ago
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a5ebc082cb2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8802899 [details] [diff] [review]
v1 (workaround ; rename LOG in PackagedAppService to LOG_PAS)

Given that this is not a new issue and we are a week away from entering RC, I am going to deny this uplift for Beta50. We should uplift to Aurora51 and let this ride the 51 train.
Attachment #8802899 - Flags: approval-mozilla-beta?
Attachment #8802899 - Flags: approval-mozilla-beta-
Attachment #8802899 - Flags: approval-mozilla-aurora?
Attachment #8802899 - Flags: approval-mozilla-aurora+

Updated

a year ago
status-firefox50: affected → wontfix
(Assignee)

Comment 8

a year ago
(In reply to Ritu Kothari (:ritu) from comment #7)
> Comment on attachment 8802899 [details] [diff] [review]
> v1 (workaround ; rename LOG in PackagedAppService to LOG_PAS)
> 
> Given that this is not a new issue and we are a week away from entering RC,
> I am going to deny this uplift for Beta50. We should uplift to Aurora51 and
> let this ride the 51 train.

I'm very vary sad from this decision, Ritu.  

This is a super simple zero-risk patch that may save a lot of headaches!  It mostly is a new issue (discovered recently) and only have impact on diagnosing problems in the most important networking part - to provide logging when turned on on-demand.  When there are problems on 50 when on release in the networking stack and we ask people for logs, we won't get the information we need.  That will be very frustrating!
(Assignee)

Comment 9

a year ago
Rita, I can't request for beta, so I'm asking for reconsideration in comment 8.
Flags: needinfo?(rkothari)
Ritu:  FWIW, this patch does seem risk-free to me. We're only changing where some output gets logged.  Having working logging makes debugging certain bugs a lot easier, so I'll give my vote for this to be on beta if possible.
Comment on attachment 8802899 [details] [diff] [review]
v1 (workaround ; rename LOG in PackagedAppService to LOG_PAS)

Based on the emphatic request from Honza and Jason, I will reconsider. Perhaps this is something where the risk is pretty low and the benefit justifiably high. Let's uplift to Beta50. This should be in 50.0b11 which we will gtb on Thursday.
Flags: needinfo?(rkothari)
Attachment #8802899 - Flags: approval-mozilla-beta- → approval-mozilla-beta+

Updated

a year ago
status-firefox50: wontfix → affected
(Assignee)

Comment 12

a year ago
Thank you, Ritu! :)

Comment 13

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1280f78d221d
status-firefox51: affected → fixed

Comment 14

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/892beac44c85
status-firefox50: affected → fixed

Updated

a year ago
status-firefox49: affected → wontfix

Updated

a year ago
Attachment #8802899 - Flags: approval-mozilla-release?
You need to log in before you can comment on or make changes to this bug.