Replace PRLogModuleInfo usage with LazyLogModule in various remaining subdirectories

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: erahm, Assigned: sajitk)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox47 fixed, firefox48 fixed)

Details

Attachments

(4 attachments, 8 obsolete attachments)

Reporter

Description

4 years ago
This covers replacing the few remaining instances of PRLogModuleInfo w/ LazyLogModule in everything in various subdirectories.

Current usage:
> ./docshell/base/nsDocShell.cpp:818:    gDocShellLog = PR_NewLogModule("nsDocShell");
> ./docshell/base/nsDocShell.cpp:822:    gDocShellLeakLog = PR_NewLogModule("nsDocShellLeak");
> ./docshell/shistory/nsSHistory.cpp:67:    sLog = PR_NewLogModule("nsSHistory");
> ./extensions/auth/nsAuthFactory.cpp:229:  gNegotiateLog = PR_NewLogModule("negotiateauth");
> ./extensions/gio/nsGIOProtocolHandler.cpp:914:  sGIOLog = PR_NewLogModule("gio");
> ./extensions/pref/autoconfig/src/nsReadConfig.cpp:79:      MCD = PR_NewLogModule("MCD");
> ./hal/Hal.cpp:69:    sHalLog = PR_NewLogModule("hal");
> ./ipc/chromium/src/base/logging.cc:66:    gChromiumPRLog = PR_NewLogModule("chromium");
> ./js/xpconnect/loader/mozJSComponentLoader.cpp:201:        gJSCLLog = PR_NewLogModule("JSComponentLoader");
> ./js/xpconnect/src/nsXPConnect.cpp:237:        gJSDiagnostics = PR_NewLogModule("JSDiagnostics");
> ./js/xpconnect/src/XPCLog.cpp:32:    g_LogMod = PR_NewLogModule("xpclog");
> ./modules/libjar/nsJARChannel.cpp:209:        gJarProtocolLog = PR_NewLogModule("nsJarProtocol");
> ./parser/htmlparser/nsExpatDriver.cpp:45:    sLog = PR_NewLogModule("expatdriver");
> ./rdf/base/nsCompositeDataSource.cpp:494:        nsRDFLog = PR_NewLogModule("RDF");
> ./rdf/base/nsInMemoryDataSource.cpp:748:        gLog = PR_NewLogModule("InMemoryDataSource");
> ./rdf/base/nsRDFContentSink.cpp:295:        gLog = PR_NewLogModule("nsRDFContentSink");
> ./rdf/base/nsRDFService.cpp:752:        gLog = PR_NewLogModule("nsRDFService");
> ./rdf/base/nsRDFXMLDataSource.cpp:401:        gLog = PR_NewLogModule("nsRDFXMLDataSource");
> ./security/certverifier/CertVerifier.cpp:55:    gCertVerifierLog = PR_NewLogModule("certverifier");
> ./security/manager/ssl/CertBlocklist.cpp:130:    gCertBlockPRLog = PR_NewLogModule("CertBlock");
> ./security/manager/ssl/nsNSSComponent.cpp:235:    gPIPNSSLog = PR_NewLogModule("pipnss");
> ./security/manager/ssl/nsNTLMAuthModule.cpp:37:    sNTLMLog = PR_NewLogModule("NTLM");
> ./security/manager/ssl/nsSecureBrowserUIImpl.cpp:121:    gSecureDocLog = PR_NewLogModule("nsSecureBrowserUI");
> ./security/manager/ssl/nsSecurityHeaderParser.cpp:54:    sSHParserLog = PR_NewLogModule("nsSecurityHeaderParser");
> ./security/manager/ssl/nsSiteSecurityService.cpp:48:    gSSSLog = PR_NewLogModule("nsSSService");
> ./security/manager/ssl/PublicKeyPinningService.cpp:27:  PR_NewLogModule("PublicKeyPinningService");
> ./security/manager/ssl/RootCertificateTelemetryUtils.cpp:17:  PR_NewLogModule("PublicKeyPinningTelemetryService");
> ./storage/mozStorageConnection.cpp:737:    gStorageLog = ::PR_NewLogModule("mozStorage");
> ./toolkit/components/downloads/ApplicationReputation.cpp:1133:    prlog = PR_NewLogModule("ApplicationReputation");
> ./toolkit/components/filewatcher/NativeFileWatcherWin.cpp:139:    gNativeWatcherPRLog = PR_NewLogModule("NativeFileWatcherService");
> ./toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1120:    gUrlClassifierDbServiceLog = PR_NewLogModule("UrlClassifierDbService");
> ./toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp:41:    gUrlClassifierPrefixSetLog = PR_NewLogModule("UrlClassifierPrefixSet");
> ./toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:44:    gUrlClassifierStreamUpdaterLog = PR_NewLogModule("UrlClassifierStreamUpdater");
> ./toolkit/components/url-classifier/ProtocolParser.cpp:81:      PR_NewLogModule("UrlClassifierProtocolParser");
> ./toolkit/xre/nsUpdateDriver.cpp:79:    sUpdateLog = PR_NewLogModule("updatedriver");
> ./uriloader/base/nsDocLoader.cpp:119:      gDocLoaderLog = PR_NewLogModule("DocLoader");
> ./uriloader/base/nsURILoader.cpp:758:    mLog = PR_NewLogModule("URILoader");
> ./uriloader/exthandler/nsExternalHelperAppService.cpp:659:    mLog = PR_NewLogModule("HelperAppService");
> ./uriloader/prefetch/nsOfflineCacheUpdateService.cpp:299:        gOfflineCacheUpdateLog = PR_NewLogModule("nsOfflineCacheUpdate");
> ./uriloader/prefetch/nsPrefetchService.cpp:337:        gPrefetchLog = PR_NewLogModule("nsPrefetch");
Assignee

Updated

4 years ago
Assignee: nobody → sajitk
Assignee

Comment 1

4 years ago
Posted patch 1219482.Part1.patch (obsolete) — Splinter Review
Attachment #8693401 - Flags: review?(benjamin)
Assignee

Comment 2

4 years ago
Posted patch 1219482.Part1.patch (obsolete) — Splinter Review
Attachment #8693401 - Attachment is obsolete: true
Attachment #8693401 - Flags: review?(benjamin)
Attachment #8693402 - Flags: review?(benjamin)
Assignee

Comment 3

4 years ago
Posted patch 1219482.Part2.patch (obsolete) — Splinter Review
Attachment #8693403 - Flags: review?(nfroyd)
Assignee

Comment 4

4 years ago
Posted patch 1219482.Part3.patch (obsolete) — Splinter Review
Attachment #8693404 - Flags: review?(dougt)
Assignee

Comment 5

4 years ago
Posted patch 1219482.Part4.patch (obsolete) — Splinter Review
Attachment #8693406 - Flags: review?(erahm)
Attachment #8693403 - Flags: review?(nfroyd) → review+
Reporter

Comment 6

4 years ago
Comment on attachment 8693406 [details] [diff] [review]
1219482.Part4.patch

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

Thank you for the patch. r=me
Attachment #8693406 - Flags: review?(erahm) → review+
Assignee

Updated

4 years ago
Attachment #8693402 - Flags: review?(benjamin) → review?(nfroyd)
Assignee

Updated

4 years ago
Attachment #8693404 - Flags: review?(dougt) → review?(erahm)
Reporter

Comment 7

4 years ago
Comment on attachment 8693404 [details] [diff] [review]
1219482.Part3.patch

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

r=me
Attachment #8693404 - Flags: review?(erahm) → review+

Updated

4 years ago
Attachment #8693402 - Flags: review?(nfroyd) → review+
Assignee

Comment 11

4 years ago
Hi, I am getting test failures related to these changes, and they seem to be related to this one in particular:
> ./toolkit/xre/nsUpdateDriver.cpp:79:    sUpdateLog = PR_NewLogModule("updatedriver");

The test failures are triggered by an assertion failure in LogModule::Get() function 
>  MOZ_ASSERT(sLogModuleManager != nullptr);

This implies that the LogModule::Init() is not getting called in the tests.
For example, the test toolkit/mozapps/update/tests/unit_base_updater/marAppApplyUpdateStageSuccess fails and the stack trace is
>#0  mozilla::LogModule::Get (aName=0x7fffebb6959c "updatedriver")
>    at /home/firefox-dev/mozilla-central/xpcom/base/Logging.cpp:80
>#1  0x00007fffe4fbb341 in mozilla::LazyLogModule::operator mozilla::LogModule*
>    (this=0x7fffee85e960 <GetUpdateLog()::sUpdateLog>)
>    at ../../dist/include/mozilla/Logging.h:117
>#2  0x00007fffe9343d0d in GetUpdateLog ()
>    at /home/firefox-dev/mozilla-central/toolkit/xre/nsUpdateDriver.cpp:78
>#3  0x00007fffe9344ee4 in SwitchToUpdatedApp (greDir=0x7ffff6b52900, 
>    updateDir=0x7ffff6b547c0, appDir=0x7ffff6b54700, appArgc=1, 
>    appArgv=0x7ffff6b457d0)
>    at /home/firefox-dev/mozilla-central/toolkit/xre/nsUpdateDriver.cpp:633
>#4  0x00007fffe9345cf7 in ProcessUpdates (greDir=0x7ffff6b52900, 
>    appDir=0x7ffff6b54700, updRootDir=0x7ffff6b54640, argc=1, 
>    argv=0x7ffff6b457d0, appVersion=0x7ffff6b7f340 "45.0a1", restart=true, 
>    isOSUpdate=false, osApplyToDir=0x0, pid=0x0)
>    at /home/firefox-dev/mozilla-central/toolkit/xre/nsUpdateDriver.cpp:1043
>#5  0x00007fffe9339e0e in XREMain::XRE_mainStartup (this=0x7fffffffc890, 
>    aExitFlag=0x7fffffffc802)
>    at /home/firefox-dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:3875
>#6  0x00007fffe933bfd2 in XREMain::XRE_main (this=0x7fffffffc890, argc=1, 
>    argv=0x7fffffffdd98, aAppData=0x7fffffffcab0)
>    at /home/firefox-dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4368
>#7  0x00007fffe933c3c6 in XRE_main (argc=1, argv=0x7fffffffdd98, 
>    aAppData=0x7fffffffcab0, aFlags=0)
>    at /home/firefox-dev/mozilla-central/toolkit/xre/nsAppRunner.cpp:4485
>#8  0x0000000000405894 in do_main (argc=1, argv=0x7fffffffdd98, 
>    xreDirectory=0x7ffff6b52900)
>    at /home/firefox-dev/mozilla-central/browser/app/nsBrowserApp.cpp:212
>#9  0x0000000000405c93 in main (argc=1, argv=0x7fffffffdd98)
>    at /home/firefox-dev/mozilla-central/browser/app/nsBrowserApp.cpp:352
>
There are no calls to LogModule::Init in nsBrowserApp.cpp or nsApprunner.cpp

Is a further fix then needed, that the LogModule::Init be put in all application entry paths? Or at least the places where NS_LogInit() is called?

Appreciate the help.
Flags: needinfo?(nfroyd)
I'm guessing that this bit of code is getting called even prior to LogModule::Init(), because XPCOM has not been initialized at this point in the code.  Maybe calling LogModule::Init() hereabouts:

https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4332

You'll notice that the call to XRE_mainStartup in the above stack is here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4368

and XPCOM doesn't start until here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4375

You may find yourself needing to add LogModule::Init() everywhere NS_LogInit is called.
Flags: needinfo?(nfroyd)
Reporter

Comment 14

4 years ago
satjik do you plan on continuing with this bug? Let us know if you have any further questions.
Flags: needinfo?(sajitk)
Assignee

Comment 15

4 years ago
Hi, 
Unfortunately, I have not been able to get the build working for this particular instance of the PR_NewLogModule use:
> ./toolkit/xre/nsUpdateDriver.cpp:79:    sUpdateLog = PR_NewLogModule("updatedriver");

With your permission, I was thinking of creating a new bug for tracking this usage, and checking all the rest in. Then someone with more knowledge about the build and link process can resolve it.

Please let me know if this is OK.
Flags: needinfo?(sajitk) → needinfo?(erahm)
Reporter

Comment 16

4 years ago
(In reply to sajitk from comment #15)
> Hi, 
> Unfortunately, I have not been able to get the build working for this
> particular instance of the PR_NewLogModule use:
> > ./toolkit/xre/nsUpdateDriver.cpp:79:    sUpdateLog = PR_NewLogModule("updatedriver");
> 
> With your permission, I was thinking of creating a new bug for tracking this
> usage, and checking all the rest in. Then someone with more knowledge about
> the build and link process can resolve it.
> 
> Please let me know if this is OK.

That sounds reasonable, thank you for working through all this!
Flags: needinfo?(erahm)
Assignee

Comment 17

3 years ago
Posted patch 1219482.Part1.patch (obsolete) — Splinter Review
Attachment #8693402 - Attachment is obsolete: true
Assignee

Comment 18

3 years ago
Posted patch 1219482.Part2.patch (obsolete) — Splinter Review
Attachment #8693403 - Attachment is obsolete: true
Assignee

Comment 19

3 years ago
Attachment #8713264 - Attachment is obsolete: true
Assignee

Comment 20

3 years ago
After fixing pull errors
Attachment #8713265 - Attachment is obsolete: true
Assignee

Comment 21

3 years ago
Removed entry in xre/toolkit causing errors.
Attachment #8693404 - Attachment is obsolete: true
Assignee

Comment 22

3 years ago
Posted patch 1219482.Part4.patch (obsolete) — Splinter Review
After resolving pull errors
Attachment #8693406 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Keywords: checkin-needed
This checkin seems to break b2g. See bug 124419.
(In reply to Hubert Figuiere [:hub] from comment #26)
> This checkin seems to break b2g. See bug 124419.

That bug seems to be about windows scrolling or form control issues?
Flags: needinfo?(hub)
I meant bug 1244149 - not enough digits, but it was mentioned above without comments. "Depends on"
Flags: needinfo?(hub)
When you reland, please push bug 1244149 at the same time, or fold it into part 4. Thanks!
Assignee

Comment 31

3 years ago
Attachment #8713272 - Attachment is obsolete: true
Reporter

Comment 34

3 years ago
Latest try build doesn't have hazard issues, I'm going to try relanding.
Reporter

Comment 35

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/df2083106d8d69bcb8112ab95efe814103947ad9
Bug 1219482: Replace PRLogModuleInfo with LazyLogModule in various files.r=benjamin

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c19306be55e240d32c7b36f39a06b640c69fce5
Bug 1219482: Replace PRLogModuleInfo with LazyLogModule in security subdirectory.r=nfroyd

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbf8a036b71960beecf26f942cb960c69af95d9
Bug 1219482: Replace PRLogModuleInfo with LazyLogModule in toolkit subdirectory.r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/602a58ea423e0691f6cdc702dee98b318b91b485
Bug 1219482: Replace PRLogModuleInfo with LazyLogModule in uriloader subdirectory.r=erahm

Updated

3 years ago
Blocks: 1256302
You need to log in before you can comment on or make changes to this bug.