XPCOM component (libdbusservice.so) is not registered although it should be

RESOLVED FIXED in mozilla10

Status

()

Core
Networking
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Martin Stránský, Assigned: Martin Stránský)

Tracking

Trunk
mozilla10
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 17 obsolete attachments)

8.70 KB, patch
Takanori MATSUURA
: review+
Biesinger
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
libdbusservice.so is not registered as XPCOM component at trunk (beta9). It's listen in components.manifest as a binary component, the library is loaded, 
ContractIDs is added in nsComponentManagerImpl::RegisterModule() but the component is still unregistered. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=620472#c36
(Assignee)

Updated

7 years ago
Assignee: nobody → stransky
blocking2.0: --- → ?

Comment 1

7 years ago
When you say "still unregistered", what are the precise symptoms?
(Assignee)

Comment 2

7 years ago
Ahh, sory, it's unregistered only in nsIOService::Init(), because it's registered later. 

So we need to register @mozilla.org/network/network-link-service;1 before "@mozilla.org/network/io-service;1" or move mNetworkLinkService initialization somewhere else.
(Assignee)

Comment 3

7 years ago
"@mozilla.org/network/io-service;1" initialization seems to come from en-US.manifest, from:

override chrome://global/locale/netError.dtd chrome://browser/locale/netError.dtd

Comment 4

7 years ago
It's not the *registration* order, precisely: it's the order in which the IO service is initialized relative to the registration of the DBUS service. In this case, the IO service init is required in order to process manifest files, so we do need to initialize the network link service lazily.
Component: XPCOM → Networking
QA Contact: xpcom → networking
(Assignee)

Comment 5

7 years ago
Created attachment 506368 [details] [diff] [review]
wip, untested

Comment 6

7 years ago
>+    if (XRE_GetProcessType() == GeckoProcessType_Default)
>+#endif
>+        mNetworkLinkService = do_GetService(NS_NETWORK_LINK_SERVICE_CONTRACTID, &rv);
>+        if (NS_FAILED(rv)) {
>+            NS_WARNING("failed to get network link service");
>+        }
>+

You should brace the if here to encompass the new addition.
Not holding the release for this, but I'd approve a safe and reviewed patch.
blocking2.0: ? → -
(Assignee)

Comment 8

7 years ago
Created attachment 507125 [details] [diff] [review]
v2

Tested one, fixed braces in nsIOService::Init(), fixed observer topic (NS_XPCOM_STARTUP_OBSERVER_ID has been removed in ff4).
Attachment #506368 - Attachment is obsolete: true
Attachment #507125 - Flags: review?(josh)

Comment 9

7 years ago
Comment on attachment 507125 [details] [diff] [review]
v2

I'm not a reviewer for this code, I'm just picking some nits as I see them.  Speaking of which, you don't need to initialize mNetworkLinkService in the constructor; nsCOMPtr already does that.
Attachment #507125 - Flags: review?(josh)
(Assignee)

Updated

7 years ago
Attachment #507125 - Flags: review?(jst)
Comment on attachment 507125 [details] [diff] [review]
v2

Dan, can you look through this? I'm no necko peer...
Attachment #507125 - Flags: review?(jst) → review?(dwitte)

Comment 11

7 years ago
Comment on attachment 507125 [details] [diff] [review]
v2

This looks fine, but I'm not sure if it will have side-effects -- does anyone expect the linkservice to be available before profile-after-change? Does anyone depend on the notifications and side-effects SetOffline() or TrackNetworkLinkStatusForOffline() cause before that point?

If you can stick some assertions around to prove that, at the least, IsLinkUp() or TrackNetworkLinkStatusForOffline() aren't called before that point, I'd feel better.

Canceling review pending that.
Attachment #507125 - Flags: review?(dwitte)
(Assignee)

Comment 12

7 years ago
Created attachment 508742 [details] [diff] [review]
v3

You're right, the mNetworkLinkService is used from nsIOService::SetManageOfflineStatus() when browser starts. This patch should handle it + there are some asserts.
Attachment #507125 - Attachment is obsolete: true
Attachment #508742 - Flags: review?(dwitte)

Comment 13

7 years ago
Comment on attachment 508742 [details] [diff] [review]
v3

> NS_IMETHODIMP
> nsIOService::SetManageOfflineStatus(PRBool aManage) {
>     PRBool wasManaged = mManageOfflineStatus;
>     mManageOfflineStatus = aManage;
>+    nsresult rv = NS_OK;
>+
>+    if (!mNetworkLinkServiceInitialized)
>+        rv = InitializeNetworkLinkService();
>+    
>     if (mManageOfflineStatus && !wasManaged)
>         return TrackNetworkLinkStatusForOffline();

This looks a little odd that you're not immediately returning on InitializeNetworkLinkService() failure. In this case TrackNetworkLinkStatusForOffline() will do that for you, but it's not obvious from this code alone. Put an explicit return for the failure case?

Otherwise looks good, r=dwitte. Is this adequately covered by existing tests?
Attachment #508742 - Flags: review?(dwitte) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 510968 [details] [diff] [review]
v4, fixed nsIOService::SetManageOfflineStatus()

Fixed nsIOService::SetManageOfflineStatus(). To be honest I have very little knowledge about mozilla tests.
Attachment #508742 - Attachment is obsolete: true
Attachment #510968 - Flags: review?(dwitte)
(Assignee)

Comment 15

7 years ago
Created attachment 510985 [details] [diff] [review]
v5

There's more compatible version attached, it leaves the SetOffline(PR_FALSE) in
nsIOService::Init() so the default initialization steps are exactly the same as w/o the patch.
Attachment #510968 - Attachment is obsolete: true
Attachment #510985 - Flags: review?(dwitte)
Attachment #510985 - Flags: approval2.0?
Attachment #510968 - Flags: review?(dwitte)
Comment on attachment 510985 [details] [diff] [review]
v5

Please only request approval once a patch is reviewed and ready to land.
Attachment #510985 - Flags: approval2.0?

Comment 17

7 years ago
Comment on attachment 510985 [details] [diff] [review]
v5

Clearing review -- I don't have time to do this in the near future. :(

Try mayhemer, jduell, or biesi?
Attachment #510985 - Flags: review?(dwitte)
(Assignee)

Updated

7 years ago
Attachment #510985 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

7 years ago
Attachment #510985 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
Note: this will get NM 0.8 working.  We'll also need bug 639959 to land in order to get NM 0.9 working.
Blocks: 639959
Whiteboard: not-ready
(Assignee)

Comment 19

7 years ago
To be clear, w/o the SetOffline(PR_FALSE); in nsIOService::Init() firefox fails to get updates for installed extensions and bothers users with "Can I go to Online now?" dialog. 

It happens only when the extensions are checked, e.g. when you update your ff version. It's fixed on the v5 patch.
Created attachment 525008 [details] [diff] [review]
interdiff between patch v4 and patch v5

In case this helps speed things along, here's an interdiff between patch v4 which already has r=dwitte, and v5.
Attachment #510985 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

6 years ago
Attachment #510985 - Flags: approval2.0?
(Assignee)

Updated

6 years ago
Whiteboard: not-ready
Can this land on mozilla-central now?
Yeah, I think this is ready to go land.
Keywords: checkin-needed
Patch v5 doesn't apply cleanly any more.  :(
Keywords: checkin-needed
(Assignee)

Comment 24

6 years ago
Created attachment 525640 [details] [diff] [review]
unbitrotten v5 patch
(Assignee)

Updated

6 years ago
Attachment #525640 - Attachment description: unbitrotten patch → unbitrotten v5 patch
(Assignee)

Updated

6 years ago
Attachment #525640 - Flags: review?(cbiesinger)
Comment on attachment 525640 [details] [diff] [review]
unbitrotten v5 patch

If it's just updating for bitrot as is the case here (MOZ_IPC was removed)... no need to ask for re-review, just carry over r=biesi.
Attachment #525640 - Flags: review?(cbiesinger) → review+
Keywords: checkin-needed
Martin, it would be really nice if you could put up a patch as described on <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f>.
(Assignee)

Comment 27

6 years ago
Created attachment 525954 [details] [diff] [review]
requested "hg commit friendly" patch

Here we come.
Thanks a lot! I expect to land it tomorrow.
http://hg.mozilla.org/projects/cedar/rev/3c42531f4ec8
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla6
http://hg.mozilla.org/mozilla-central/rev/3c42531f4ec8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
We suspect that this might have caused our Maemo tests going orange, so I backed it out to test this assumption:

http://hg.mozilla.org/mozilla-central/rev/d492992e2884

If I'm wrong, I will reland it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backing this patch out fixed the mobile tree.

Please talk to the mobile people on how you can fix this issue before we attempt to reland this patch on mozilla-central.
(Assignee)

Comment 33

6 years ago
Who are "the mobile people"?
The folks ehsan CCd.
(Assignee)

Comment 35

6 years ago
Aha, okay. So can I find some closer info somewhere? About the Maemo failure?
The failures can be seen here:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mobile&maxdate=1302915658&hours=24

n900-gtk mozilla-central unit mochitest1
n900-gtk mozilla-central unit mochitest2
n900-gtk mozilla-central unit mochitest3
n900-gtk mozilla-central unit mochitest4

These tests were all orange until the backout (moz:d492992e2884)
I think we should not enable libdbusservice for maemo, because on maemo we initializing dbus service context in osso_init function (at startup), and I guess problem appearing when libdbusservice initializing another default dbus session, so we getting some conflict and world is broken....
dbus service did not work for maemo before and don't see any reason enable it now.
So better to disable that registration in init function or just disable that component for maemo completely.
(Assignee)

Comment 38

6 years ago
Created attachment 530257 [details] [diff] [review]
disabled libdbusservice on Maemo

The same patch, but libdbusservice is disabled on Maemo platform.
Attachment #525954 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Status: REOPENED → ASSIGNED
From patch it looks like you are disabling networklink service instead of libdbus service... why ? or I overlooked something?
(Assignee)

Comment 40

6 years ago
Ahh, sorry, it's a typo in patch description. It disables the NS_NETWORK_LINK_SERVICE for Maeno.
why you disabling that? it is actually used, and working
(Assignee)

Comment 42

6 years ago
(In reply to comment #41)
> why you disabling that? it is actually used, and working

Really? Without this patch, mNetworkLinkService is NULL and is disabled, at least on Linux platform, because of wrong components registration order (see https://bugzilla.mozilla.org/show_bug.cgi?id=620472#c36).

Does Maemo use different component registration order?
On maemo5 we are using:
mozilla-central/netwerk/system/maemo
maemo6:
                netwerk/system/qt
and that is working in order to make link service working for Autodial functionality,, raising connection when it is not available...
see https://bugzilla.mozilla.org/show_bug.cgi?id=646412 and related bugs
(Assignee)

Comment 44

6 years ago
Okay, I installed the Maemo dev environment and I'm building it in scratchbox right now. But is the latest trunk supposed to build there anyway? I'm getting link/relocating errors like:

../xre/nsEmbedFunctions.o: In function `XRE_InitChildProcess':
/home/komat/src/toolkit/xre/nsEmbedFunctions.cpp:420: relocation truncated to fit: R_ARM_PLT32 against symbol `base::OpenProcessHandle(int, int*)' defined in .text section in ../../ipc/chromium/process_util_posix.o

and so...

The mozconfig i use comes from https://wiki.mozilla.org/Mobile/Build/Fennec
Comment on attachment 510985 [details] [diff] [review]
v5

really needed on the now-dead mozilla2.0 branch? (mozilla2.1 is mobile's branch). Anyway, not approving this patch because nothing has landed successfully on trunk yet.
Attachment #510985 - Flags: approval2.0? → approval2.0-
(In reply to comment #45)
> Comment on attachment 510985 [details] [diff] [review] [review]
> v5
> 
> really needed on the now-dead mozilla2.0 branch? (mozilla2.1 is mobile's
> branch). Anyway, not approving this patch because nothing has landed
> successfully on trunk yet.

This should only need to be landed on Trunk (Fx6)
(Assignee)

Comment 47

6 years ago
The maemo build fails with disabled optimalizations and enabled debug. You guys should update the wiki page.
(Assignee)

Comment 48

6 years ago
(In reply to comment #37)
> I think we should not enable libdbusservice for maemo, because on maemo we
> initializing dbus service context in osso_init function (at startup), and I
> guess problem appearing when libdbusservice initializing another default
> dbus session, so we getting some conflict and world is broken....
> dbus service did not work for maemo before and don't see any reason enable
> it now.

libdbusservice is already disabled on maemo, it's built with MOZ_ENABLE_LIBCONIC. (see toolkit/toolkit-tiers.mk).

Unfortunately the mochitest logs does not contain any explicit error. My test fenec builds run fine regardless the patch.

And maemo has different components registration order so it does not need this patch.
(Assignee)

Updated

6 years ago
Attachment #530257 - Attachment is obsolete: true
(Assignee)

Comment 49

6 years ago
Created attachment 533951 [details] [diff] [review]
v6, fix for maemo mochitest

The mochitest fails because it calls SetManageOfflineStatus() early, the InitializeNetworkLinkService() fails and error is returned to mochitest.

This patch removes the error propagation (like without the patch) so it fails silently and InitializeNetworkLinkService() is called later.
Attachment #510985 - Attachment is obsolete: true
Attachment #525008 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #533951 - Attachment description: fix for maemo mochitest → v6, fix for maemo mochitest
(Assignee)

Comment 50

6 years ago
Created attachment 533954 [details] [diff] [review]
diff between v5 and v6
(Assignee)

Updated

6 years ago
Attachment #533954 - Flags: review?(cbiesinger)
(Assignee)

Updated

6 years ago
Attachment #533954 - Flags: review?(cbiesinger)
(Assignee)

Comment 51

6 years ago
Comment on attachment 533951 [details] [diff] [review]
v6, fix for maemo mochitest

Can you check it please?
Attachment #533951 - Flags: review?(cbiesinger)
(Assignee)

Updated

6 years ago
Attachment #533951 - Flags: review?(cbiesinger) → review?(honzab.moz)

Comment 52

6 years ago
(In reply to Martin Stránský from comment #51)

Applying Attachment 533951 [details] [diff] is failed by hg:1d1143dde4bb
http://hg.mozilla.org/mozilla-central/rev/1d1143dde4bb

Comment 53

6 years ago
Created attachment 563369 [details] [diff] [review]
v6.1, fix for maemo mochitest (fit to the latest m-c)

Just fit attachment 533951 [details] [diff] [review] to the latest m-c.

I cannot understand how the patch fix this bug.
So I hope Martin will follow up.
Attachment #533951 - Attachment is obsolete: true
Attachment #533954 - Attachment is obsolete: true
Attachment #563369 - Flags: feedback?(stransky)
Attachment #533951 - Flags: review?(honzab.moz)

Comment 54

6 years ago
Created attachment 563370 [details] [diff] [review]
fit coding style to the latest one

Fix only the coding style to the latest one.
No string changed.
(Assignee)

Comment 55

6 years ago
(In reply to Takanori MATSUURA from comment #53)
> I cannot understand how the patch fix this bug.
> So I hope Martin will follow up.

See comment 4. Without this patch, nsIOService::Init() fails to set up the mNetworkLinkService (NS_NETWORK_LINK_SERVICE_CONTRACTID is unknown at this moment) so mNetworkLinkService is NULL for rest of the time.

This patch initializes mNetworkLinkService lazily, i.e. when all components are initialized and it uses the "profile-after-change" observer event for it.
(Assignee)

Updated

6 years ago
Attachment #563369 - Flags: feedback?(stransky) → feedback+
(Assignee)

Updated

6 years ago
Attachment #563369 - Flags: review?(cbiesinger)
(Assignee)

Updated

6 years ago
Attachment #533954 - Attachment is obsolete: false
Comment on attachment 563369 [details] [diff] [review]
v6.1, fix for maemo mochitest (fit to the latest m-c)

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

(r-) based on comments.

Sorry for taking it almost 4 months to take a look at this.  I was really busy.

::: netwerk/base/src/nsIOService.cpp
@@ +163,5 @@
>  };
>  
>  static const char kProfileChangeNetTeardownTopic[] = "profile-change-net-teardown";
>  static const char kProfileChangeNetRestoreTopic[] = "profile-change-net-restore";
> +static const char kStartupTopic[] = "profile-after-change";

this has to be kProfileAfterChangeTopic

I think you may rather want to watch for "profile-do-change" with data == L"startup".

@@ +179,5 @@
>      , mManageOfflineStatus(PR_TRUE)
>      , mSettingOffline(PR_FALSE)
>      , mSetOfflineValue(PR_FALSE)
>      , mShutdown(PR_FALSE)
> +    , mNetworkLinkServiceInitialized(PR_FALSE)

use |false| - we have migrated to using |bool| only

@@ +266,5 @@
>      gIOService = this;
>  
> +    // We can't really determine if the machine has a usable network connection,
> +    // (mNetworkLinkService will be initialized later) so let's cross our fingers!
> +    SetOffline(PR_FALSE);

I'd leave this code as is.

Or better:

- move this code to a new method (e.g. InitializeNetworkLinkService()) and leave it as is with:
- early return from the method on mNetworkLinkServiceInitialized == true
- early return when not on the main thread (link service should be created only on the main thread, it is using observer service) and warn we are trying to create the link service on non-main thread

- call the method here, if the service was instantiated, set the mNetworkLinkServiceInitialized to true
- call it in nsIOService::IsLinkUp, don't modify the flag
- call it from "profile-do-change" observer code and also set the flag mNetworkLinkServiceInitialized to true, nevertheless the service succeeded or failed to init (if it failed on this place, there is probably no chance to succeed on any later attempt)


I wouldn't call it from kProfileChangeNetRestoreTopic because there is not much of a chance to get that topic called between instantiating IOService and profile-do-change.  The code that can invoke this topic is instantiated earlier then IOService (see http://mxr.mozilla.org/mozilla-central/ident?i=NS_CreateNativeAppSupport) but it could be called sooner the link service is able to instantiate.  But see bug 679733 for more info.

@@ +314,5 @@
> +      return rv;
> +
> +#if defined(MOZ_PLATFORM_MAEMO)
> +    // libdbusservice fails to initialize on Maemo platform, see Bug 627672
> +    mNetworkLinkService = NULL;

Why this change?  It is not clear to me from the comments why just bypass the creation.  If this is because the failing test, then the test needs to be changed.

@@ +322,5 @@
> +    {
> +        mNetworkLinkService = do_GetService(NS_NETWORK_LINK_SERVICE_CONTRACTID, &rv);
> +        if (NS_FAILED(rv)) {
> +            NS_WARNING("failed to get network link service");
> +            return rv;

Why hard fail?  With the code logic as is, we will try this on and on... wasting...  also, who sets the mManageOfflineStatus to false?

@@ +327,5 @@
> +        }
> +    }
> +#endif
> +
> +    mNetworkLinkServiceInitialized = PR_TRUE;

true

@@ +332,5 @@
> +
> +    if (!mNetworkLinkService) {
> +        // We can't really determine if the machine has a usable network connection,
> +        // so let's cross our fingers!
> +        mManageOfflineStatus = PR_FALSE;

false

@@ +724,5 @@
>  bool
>  nsIOService::IsLinkUp()
>  {
> +    NS_ASSERTION(mNetworkLinkServiceInitialized, 
> +                 "network link service should be initialized");

As said above, rather try to instantiate the service here.

@@ +1007,5 @@
>          } 
>      }
> +    else if (!strcmp(topic, kStartupTopic)) {
> +        // Lazy initialization of network link service (see bug 620472)
> +        InitializeNetworkLinkService();

And always set the flag to true..

@@ +1129,5 @@
>      mManageOfflineStatus = aManage;
> +
> +    if (!mNetworkLinkServiceInitialized) {
> +        InitializeNetworkLinkService();
> +    }

No need for this code here.  Also the behavior now is to throw if we cannot manage (we always have to call TrackNetworkLinkStatusForOffline ; with your change mManageOfflineStatus will drop back to false when init of the link service fail, and we would then falsely return NS_OK, although we don't manage!)

If the mobile test fails because of this method exception, then change the test.

@@ +1135,2 @@
>      if (mManageOfflineStatus && !wasManaged)
>          return TrackNetworkLinkStatusForOffline();

Rather call InitializeNetworkLinkService() here, in this branch, before TrackNetworkLinkStatusForOffline() and always set the flag to true.
Attachment #563369 - Flags: review?(cbiesinger)
(In reply to Honza Bambas (:mayhemer) from comment #56)
> @@ +1135,2 @@
> >      if (mManageOfflineStatus && !wasManaged)
> >          return TrackNetworkLinkStatusForOffline();
> 
> Rather call InitializeNetworkLinkService() here, in this branch, before
> TrackNetworkLinkStatusForOffline() and always set the flag to true.

This should be 'set the flag when instantiation succeeded'.
(Assignee)

Comment 58

6 years ago
Created attachment 566814 [details] [diff] [review]
v7

Thanks for the review, this one should address your comments.
Attachment #525640 - Attachment is obsolete: true
Attachment #533954 - Attachment is obsolete: true
Attachment #563369 - Attachment is obsolete: true
Attachment #563370 - Attachment is obsolete: true
Attachment #566814 - Flags: review?(honzab.moz)
Comment on attachment 566814 [details] [diff] [review]
v7

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

r- for the attribute behavior change.

Roc, if you think the attribute should not throw when failed to set offline manage, then let us know.

::: netwerk/base/src/nsIOService.cpp
@@ +309,5 @@
> +
> +    nsresult rv = NS_OK;
> +
> +    if (mNetworkLinkServiceInitialized)
> +        return rv;

Maybe return NS_OK and declare |rv| later?  Up to you.

@@ +322,5 @@
> +    {
> +        mNetworkLinkService = do_GetService(NS_NETWORK_LINK_SERVICE_CONTRACTID, &rv);
> +    }
> +
> +    mManageOfflineStatus = (bool)mNetworkLinkService;

You must not set mManageOfflineStatus to true if you just succeeded to create the link service.  Preserve please the previous code (only drop to false on a failure).

@@ +712,5 @@
>  
>  bool
>  nsIOService::IsLinkUp()
>  {
> +    InitializeNetworkLinkService();

I don't remember the reason to not modify the init flag here..  If we would get here sooner then profile-do-change fired, we could execute code of InitializeNetworkLinkService() 2 or more times... maybe rather manage the flag in the method and only in profile-do-change set it true nevertheless the result of initiation?

@@ +985,5 @@
>              mOfflineForProfileChange = PR_TRUE;
>          }
>      }
> +    else if (!strcmp(topic, kProfileDoChange)) { 
> +        if(data && NS_LITERAL_STRING("startup").Equals(data)) {

Space between if and (

Maybe have just a single && condition?

@@ -966,5 @@
> -            if (!mManageOfflineStatus ||
> -                NS_FAILED(TrackNetworkLinkStatusForOffline())) {
> -                SetOffline(PR_FALSE);
> -            }
> -        } 

Why are you removing the handler for kProfileChangeNetRestoreTopic?  My comment was just about not calling the InitializeNetworkLinkService() method from there, not to remove the whole code branch for it!

@@ +1116,2 @@
>      if (mManageOfflineStatus && !wasManaged)
>          return TrackNetworkLinkStatusForOffline();

This code is wrong IMO.  I still think the attribute should throw when we are not able to turn manage on (i.e. we don't have the link service) and are before profile-do-change.  

To fix the code as I think is correct would be to do mManageOfflineStatus = aManage; *after* call to InitializeNetworkLinkService().  This way we will throw when there is no link service.

@@ +1129,5 @@
>  {
>      NS_ASSERTION(mManageOfflineStatus,
>                   "Don't call this unless we're managing the offline status");
> +    NS_ASSERTION(mNetworkLinkService,
> +                "network link service should be set up");

If we decide to change the attribute as I suggested, this assertion should then go away.
Attachment #566814 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #59)
> @@ +1116,2 @@
> >      if (mManageOfflineStatus && !wasManaged)
> >          return TrackNetworkLinkStatusForOffline();
> 
> This code is wrong IMO.  I still think the attribute should throw when we
> are not able to turn manage on (i.e. we don't have the link service) and are
> before profile-do-change.  
> 
> To fix the code as I think is correct would be to do mManageOfflineStatus =
> aManage; *after* call to InitializeNetworkLinkService().  This way we will
> throw when there is no link service.
> 

And also probably drop the mManageOfflineStatus flag on failure of TrackNetworkLinkStatusForOffline()..
(Assignee)

Comment 61

6 years ago
Created attachment 567073 [details] [diff] [review]
v8

>>@@ +1116,2 @@
>>      if (mManageOfflineStatus && !wasManaged)
>>          return TrackNetworkLinkStatusForOffline();

> This code is wrong IMO.  I still think the attribute should throw
> when we are not able to turn manage on (i.e. we don't have
> the link service) and are before profile-do-change.
> To fix the code as I think is correct would be to do
> mManageOfflineStatus = aManage; *after* call to
> InitializeNetworkLinkService().  This way we will 
> throw when there is no link service.

Don't you mean wasManaged = mManageOfflineStatus? Because set mManageOfflineStatus = aManage after InitializeNetworkLink
Service() overrides the mManageOfflineStatus.
Attachment #566814 - Attachment is obsolete: true
Attachment #567073 - Flags: review?(honzab.moz)
Comment on attachment 567073 [details] [diff] [review]
v8

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

::: netwerk/base/src/nsIOService.cpp
@@ +1003,5 @@
> +    else if (!strcmp(topic, kProfileDoChange)) { 
> +        if (data && NS_LITERAL_STRING("startup").Equals(data)) {
> +            // Lazy initialization of network link service (see bug 620472)
> +            InitializeNetworkLinkService();
> +            mNetworkLinkServiceInitialized = true;

Maybe add a comment we do this nevertheless we actually have the service or not to prevent subsequent attempts to init it.  If we fail here, we will fail always on.

@@ +1121,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsIOService::SetManageOfflineStatus(bool aManage) {
> +

No new line here.
Attachment #567073 - Flags: review?(honzab.moz)
(Assignee)

Comment 63

6 years ago
Created attachment 567090 [details] [diff] [review]
v9
Attachment #567073 - Attachment is obsolete: true
Attachment #567090 - Flags: review?(honzab.moz)
(Assignee)

Comment 64

6 years ago
Created attachment 567095 [details] [diff] [review]
v10
Attachment #567090 - Attachment is obsolete: true
Attachment #567095 - Flags: review?(honzab.moz)
Attachment #567090 - Flags: review?(honzab.moz)
Comment on attachment 567095 [details] [diff] [review]
v10

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9b9a291867d0
Comment on attachment 567095 [details] [diff] [review]
v10

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

Try has passed.

r=honzab

I will land this patch my self.
Attachment #567095 - Flags: review?(honzab.moz) → review+

Comment 67

6 years ago
Created attachment 567699 [details] [diff] [review]
v10.1

Adjust the patch to fit to the latest m-c.
Carried over r=honzab.

nsIOService.cpp has been modified by
https://hg.mozilla.org/mozilla-central/rev/ec7577dec4fc
Attachment #567095 - Attachment is obsolete: true
Attachment #567699 - Flags: review+
(In reply to Takanori MATSUURA from comment #67)
> Adjust the patch to fit to the latest m-c.
Thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c886f3cef1a9
Whiteboard: [inbound]
Attachment #567699 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c886f3cef1a9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla6 → mozilla10
Depends on: 720320
Depends on: 791626
You need to log in before you can comment on or make changes to this bug.