Comment 1 Benjamin Smedberg [:bsmedberg] 2011-01-21 04:20:48 PST

When you say "still unregistered", what are the precise symptoms?

Comment 2 Martin Stránský 2011-01-21 04:36:35 PST

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.

Comment 3 Martin Stránský 2011-01-21 04:54:25 PST

"@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 Benjamin Smedberg [:bsmedberg] 2011-01-21 06:46:58 PST

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.

Comment 5 Martin Stránský 2011-01-24 06:12:56 PST

Created attachment 506368 [details] [diff] [review]
wip, untested

Comment 6 Josh Matthews [:jdm] 2011-01-24 07:47:59 PST

>+    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.

Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2011-01-24 16:45:53 PST

Not holding the release for this, but I'd approve a safe and reviewed patch.

Comment 8 Martin Stránský 2011-01-26 07:49:15 PST

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

Comment 9 Josh Matthews [:jdm] 2011-01-26 08:15:13 PST

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.

Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2011-01-26 13:12:32 PST

Comment on attachment 507125 [details] [diff] [review]
v2

Dan, can you look through this? I'm no necko peer...

Comment 11 dwitte@gmail.com 2011-01-27 17:27:49 PST

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.

Comment 12 Martin Stránský 2011-02-01 05:55:14 PST

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.

Comment 13 dwitte@gmail.com 2011-02-08 14:45:16 PST

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?

Comment 14 Martin Stránský 2011-02-09 00:55:21 PST

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

Fixed nsIOService::SetManageOfflineStatus(). To be honest I have very little knowledge about mozilla tests.

Comment 15 Martin Stránský 2011-02-09 02:32:37 PST

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.

Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2011-02-15 12:07:47 PST

Comment on attachment 510985 [details] [diff] [review]
v5

Please only request approval once a patch is reviewed and ready to land.

Comment 17 dwitte@gmail.com 2011-03-03 00:08:06 PST

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?

Comment 18 Christopher Aillon (sabbatical, not receiving bugmail) 2011-03-21 12:24:04 PDT

Note: this will get NM 0.8 working.  We'll also need bug 639959 to land in order to get NM 0.9 working.

Comment 19 Martin Stránský 2011-04-07 07:09:22 PDT

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.

Comment 20 Christopher Aillon (sabbatical, not receiving bugmail) 2011-04-10 17:08:27 PDT

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.

Comment 21 :Ehsan Akhgari 2011-04-12 15:45:57 PDT

Can this land on mozilla-central now?

Comment 22 Christopher Aillon (sabbatical, not receiving bugmail) 2011-04-12 15:46:53 PDT

Yeah, I think this is ready to go land.

Comment 23 :Ehsan Akhgari 2011-04-12 18:03:23 PDT

Patch v5 doesn't apply cleanly any more.  :(

Comment 24 Martin Stránský 2011-04-13 01:25:50 PDT

Created attachment 525640 [details] [diff] [review]
unbitrotten v5 patch

Comment 25 Christopher Aillon (sabbatical, not receiving bugmail) 2011-04-13 10:42:15 PDT

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.

Comment 26 :Ms2ger (⌚ UTC+1/+2) 2011-04-13 12:45:09 PDT

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>.

Comment 27 Martin Stránský 2011-04-14 01:05:54 PDT

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

Here we come.

Comment 28 :Ms2ger (⌚ UTC+1/+2) 2011-04-14 03:47:10 PDT

Thanks a lot! I expect to land it tomorrow.

Comment 29 :Ehsan Akhgari 2011-04-14 11:12:44 PDT

http://hg.mozilla.org/projects/cedar/rev/3c42531f4ec8

Comment 30 :Ehsan Akhgari 2011-04-15 06:59:00 PDT

http://hg.mozilla.org/mozilla-central/rev/3c42531f4ec8

Comment 31 :Ehsan Akhgari 2011-04-15 13:43:30 PDT

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.

Comment 32 :Ehsan Akhgari 2011-04-15 15:06:20 PDT

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.

Comment 33 Martin Stránský 2011-05-04 03:56:59 PDT

Who are "the mobile people"?

Comment 34 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-04 04:20:49 PDT

The folks ehsan CCd.

Comment 35 Martin Stránský 2011-05-04 05:01:31 PDT

Aha, okay. So can I find some closer info somewhere? About the Maemo failure?

Comment 36 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-04 06:31:15 PDT

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)

Comment 37 Oleg Romashin (:romaxa) 2011-05-04 07:33:49 PDT

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.

Comment 38 Martin Stránský 2011-05-05 01:59:19 PDT

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

The same patch, but libdbusservice is disabled on Maemo platform.

Comment 39 Oleg Romashin (:romaxa) 2011-05-05 02:15:25 PDT

From patch it looks like you are disabling networklink service instead of libdbus service... why ? or I overlooked something?

Comment 40 Martin Stránský 2011-05-05 02:29:35 PDT

Ahh, sorry, it's a typo in patch description. It disables the NS_NETWORK_LINK_SERVICE for Maeno.

Comment 41 Oleg Romashin (:romaxa) 2011-05-05 02:48:45 PDT

why you disabling that? it is actually used, and working

Comment 42 Martin Stránský 2011-05-05 02:59:51 PDT

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

Comment 43 Oleg Romashin (:romaxa) 2011-05-05 15:20:58 PDT

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

Comment 44 Martin Stránský 2011-05-09 06:09:35 PDT

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 45 Daniel Veditz [:dveditz] 2011-05-11 10:56:13 PDT

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.

Comment 46 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-11 11:18:35 PDT

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

Comment 47 Martin Stránský 2011-05-12 03:02:30 PDT

The maemo build fails with disabled optimalizations and enabled debug. You guys should update the wiki page.

Comment 48 Martin Stránský 2011-05-18 08:21:57 PDT

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

Comment 49 Martin Stránský 2011-05-20 06:45:31 PDT

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.

Comment 50 Martin Stránský 2011-05-20 06:51:35 PDT

Created attachment 533954 [details] [diff] [review]
diff between v5 and v6

Comment 51 Martin Stránský 2011-05-20 06:58:42 PDT

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

Can you check it please?

Comment 52 Takanori MATSUURA 2011-09-11 07:21:12 PDT

(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 Takanori MATSUURA 2011-09-29 04:29:11 PDT

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.

Comment 54 Takanori MATSUURA 2011-09-29 04:31:48 PDT

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.

Comment 55 Martin Stránský 2011-09-29 06:33:11 PDT

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

Comment 56 Honza Bambas (:mayhemer) 2011-10-12 07:37:22 PDT

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.

Comment 57 Honza Bambas (:mayhemer) 2011-10-12 07:38:25 PDT

(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'.

Comment 58 Martin Stránský 2011-10-13 07:46:29 PDT

Created attachment 566814 [details] [diff] [review]
v7

Thanks for the review, this one should address your comments.

Comment 59 Honza Bambas (:mayhemer) 2011-10-13 10:07:00 PDT

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.

Comment 60 Honza Bambas (:mayhemer) 2011-10-14 07:21:26 PDT

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

Comment 61 Martin Stránský 2011-10-14 07:23:03 PDT

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.

Comment 62 Honza Bambas (:mayhemer) 2011-10-14 07:49:41 PDT

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.

Comment 63 Martin Stránský 2011-10-14 08:07:28 PDT

Created attachment 567090 [details] [diff] [review]
v9

Comment 64 Martin Stránský 2011-10-14 08:10:56 PDT

Created attachment 567095 [details] [diff] [review]
v10

Comment 65 Honza Bambas (:mayhemer) 2011-10-14 10:21:41 PDT

Comment on attachment 567095 [details] [diff] [review]
v10

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9b9a291867d0

Comment 66 Honza Bambas (:mayhemer) 2011-10-17 08:27:39 PDT

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.

Comment 67 Takanori MATSUURA 2011-10-18 02:57:37 PDT

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

Comment 68 Honza Bambas (:mayhemer) 2011-10-18 08:53:23 PDT

(In reply to Takanori MATSUURA from comment #67)
> Adjust the patch to fit to the latest m-c.
Thanks.

Comment 69 Honza Bambas (:mayhemer) 2011-10-18 09:28:15 PDT

https://hg.mozilla.org/integration/mozilla-inbound/rev/c886f3cef1a9

Comment 70 Marco Bonardo [::mak] 2011-10-19 03:10:35 PDT

https://hg.mozilla.org/mozilla-central/rev/c886f3cef1a9