Closed
Bug 627672
Opened 10 years ago
Closed 10 years ago
XPCOM component (libdbusservice.so) is not registered although it should be
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: stransky, Assigned: stransky)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 17 obsolete files)
8.70 KB,
patch
|
t.matsuu
:
review+
Biesinger
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Assignee: nobody → stransky
blocking2.0: --- → ?
Comment 1•10 years ago
|
||
When you say "still unregistered", what are the precise symptoms?
Assignee | ||
Comment 2•10 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•10 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•10 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•10 years ago
|
||
Comment 6•10 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.
Comment 7•10 years ago
|
||
Not holding the release for this, but I'd approve a safe and reviewed patch.
blocking2.0: ? → -
Assignee | ||
Comment 8•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #507125 -
Flags: review?(jst)
Comment 10•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 years ago
|
||
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 16•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #510985 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•10 years ago
|
Attachment #510985 -
Flags: review?(jduell.mcbugs) → review?(cbiesinger)
Comment 18•10 years ago
|
||
Note: this will get NM 0.8 working. We'll also need bug 639959 to land in order to get NM 0.9 working.
Updated•10 years ago
|
Whiteboard: not-ready
Assignee | ||
Comment 19•10 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.
Comment 20•10 years ago
|
||
In case this helps speed things along, here's an interdiff between patch v4 which already has r=dwitte, and v5.
Updated•10 years ago
|
Attachment #510985 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #510985 -
Flags: approval2.0?
Assignee | ||
Updated•10 years ago
|
Whiteboard: not-ready
Comment 21•10 years ago
|
||
Can this land on mozilla-central now?
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #525640 -
Attachment description: unbitrotten patch → unbitrotten v5 patch
Assignee | ||
Updated•10 years ago
|
Attachment #525640 -
Flags: review?(cbiesinger)
Comment 25•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 26•10 years ago
|
||
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•10 years ago
|
||
Here we come.
Comment 28•10 years ago
|
||
Thanks a lot! I expect to land it tomorrow.
Comment 29•10 years ago
|
||
http://hg.mozilla.org/projects/cedar/rev/3c42531f4ec8
Comment 30•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3c42531f4ec8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Comment 31•10 years ago
|
||
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 → ---
Comment 32•10 years ago
|
||
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•10 years ago
|
||
Who are "the mobile people"?
The folks ehsan CCd.
Assignee | ||
Comment 35•10 years ago
|
||
Aha, okay. So can I find some closer info somewhere? About the Maemo failure?
Comment 36•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
The same patch, but libdbusservice is disabled on Maemo platform.
Attachment #525954 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → ASSIGNED
Comment 39•10 years ago
|
||
From patch it looks like you are disabling networklink service instead of libdbus service... why ? or I overlooked something?
Assignee | ||
Comment 40•10 years ago
|
||
Ahh, sorry, it's a typo in patch description. It disables the NS_NETWORK_LINK_SERVICE for Maeno.
Comment 41•10 years ago
|
||
why you disabling that? it is actually used, and working
Assignee | ||
Comment 42•10 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?
Comment 43•10 years ago
|
||
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•10 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 45•10 years ago
|
||
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-
Comment 46•10 years ago
|
||
(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•10 years ago
|
||
The maemo build fails with disabled optimalizations and enabled debug. You guys should update the wiki page.
Assignee | ||
Comment 48•10 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•10 years ago
|
Attachment #530257 -
Attachment is obsolete: true
Assignee | ||
Comment 49•10 years ago
|
||
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•10 years ago
|
Attachment #533951 -
Attachment description: fix for maemo mochitest → v6, fix for maemo mochitest
Assignee | ||
Comment 50•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #533954 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•10 years ago
|
Attachment #533954 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 51•10 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•10 years ago
|
Attachment #533951 -
Flags: review?(cbiesinger) → review?(honzab.moz)
Comment 52•10 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•10 years ago
|
||
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•10 years ago
|
||
Fix only the coding style to the latest one. No string changed.
Assignee | ||
Comment 55•10 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•10 years ago
|
Attachment #563369 -
Flags: feedback?(stransky) → feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #563369 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•10 years ago
|
Attachment #533954 -
Attachment is obsolete: false
![]() |
||
Comment 56•10 years ago
|
||
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)
![]() |
||
Comment 57•10 years ago
|
||
(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•10 years ago
|
||
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 59•10 years ago
|
||
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-
![]() |
||
Comment 60•10 years ago
|
||
(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•10 years ago
|
||
>>@@ +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 62•10 years ago
|
||
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•10 years ago
|
||
Attachment #567073 -
Attachment is obsolete: true
Attachment #567090 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 64•10 years ago
|
||
Attachment #567090 -
Attachment is obsolete: true
Attachment #567095 -
Flags: review?(honzab.moz)
Attachment #567090 -
Flags: review?(honzab.moz)
![]() |
||
Comment 65•10 years ago
|
||
Comment on attachment 567095 [details] [diff] [review] v10 Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9b9a291867d0
![]() |
||
Comment 66•10 years ago
|
||
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•10 years ago
|
||
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+
![]() |
||
Comment 68•10 years ago
|
||
(In reply to Takanori MATSUURA from comment #67) > Adjust the patch to fit to the latest m-c. Thanks.
![]() |
||
Comment 69•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c886f3cef1a9
Whiteboard: [inbound]
Updated•10 years ago
|
Attachment #567699 -
Flags: review+
Comment 70•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c886f3cef1a9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla6 → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•