Closed Bug 1219482 Opened 9 years ago Closed 8 years ago

Replace PRLogModuleInfo usage with LazyLogModule in various remaining subdirectories

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: erahm, Assigned: sajitk)

References

Details

Attachments

(4 files, 8 obsolete files)

1219482.Part1.patch
35.45 KB, patch
Details | Diff | Splinter Review
1219482.Part2.patch
23.10 KB, patch
Details | Diff | Splinter Review
1219482.Part3.patch
13.61 KB, patch
Details | Diff | Splinter Review
1219482.Part4.patch
12.74 KB, patch
Details | Diff | Splinter Review 
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: nobody → sajitk
Attached patch 1219482.Part1.patch (obsolete) — — Splinter Review 

        Attachment #8693401 -
        Flags: review?(benjamin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part1.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8693401 -
        Attachment is obsolete: true

        Attachment #8693401 -
        Flags: review?(benjamin)

        Attachment #8693402 -
        Flags: review?(benjamin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part2.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8693403 -
        Flags: review?(nfroyd)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part3.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8693404 -
        Flags: review?(dougt)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part4.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8693406 -
        Flags: review?(erahm)

        Attachment #8693403 -
        Flags: review?(nfroyd) → review+

    
    

    
  
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+

    
  

        Attachment #8693402 -
        Flags: review?(benjamin) → review?(nfroyd)

    
  

        Attachment #8693404 -
        Flags: review?(dougt) → review?(erahm)

    
    

    
  
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+

    
  

        Attachment #8693402 -
        Flags: review?(nfroyd) → review+

    
    

    
  
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)

    
    

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

    
    

    
  
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)

    
    

    
  
(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)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part1.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8693402 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part2.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8693403 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8713264 -
        Attachment is obsolete: true

    
    

    
  


  
After fixing pull errors

        Attachment #8713265 -
        Attachment is obsolete: true

    
    

    
  


  
Removed entry in xre/toolkit causing errors.

        Attachment #8693404 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1219482.Part4.patch (obsolete)
        — 
          — Splinter Review
      

    


  
After resolving pull errors

        Attachment #8693406 -
        Attachment is obsolete: true

    
  
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!

    
    

    
  


  

        Attachment #8713272 -
        Attachment is obsolete: true

    
    

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

    
    

    
  
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

    
  
Blocks: 1256302

    
  
Blocks: 1257786

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: