Closed Bug 489864 Opened 13 years ago Closed 13 years ago

get rid of nsIInternetConfigService

Categories

(Core :: General, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: smichaud)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 6 obsolete files)

nsIInternetConfigService is an interface built around a deprecated and not-awesome Mac system API. We should get rid of it.
Attached patch fix v1.0 (obsolete) — Splinter Review
This does it, haven't tested thoroughly yet so not requesting review yet. Drops 1300 lines of deprecated junk. The only behavioral change worth noting is that we won't ever set type codes on downloaded files with this patch. I'd rather not do that anyway. We only did that for files that didn't have an extension and had mime types we could convert to type codes, which we can't figure out reliably anyway and it isn't our problem. There was a bunch of discussion about this in bug 236389, more time has passed and type codes are even more outdated than they were then.
This also fixes bug 488899.
Attachment #374358 - Flags: review?(smichaud)
Comment on attachment 374358 [details] [diff] [review]
fix v1.0

I haven't finished my review yet.

But I've noticed that there are a bunch of files under mailnews (in
the comm-central tree) that reference nsMIMEInfo::GetMacType(),
nsMIMEInfo::SetMacType(), nsMIMEInfo::GetMacCreator() or
nsMIMEInfo::SetMacCreator():

  mailnews/base/src/nsMessenger.cpp
  mailnews/compose/src/nsMsgAttachment.cpp
  mailnews/compose/src/nsMsgSend.cpp
  mailnews/mime/src/mimedrft.cpp

So this patch will presumably break Thunderbird and/or Seamonkey
builds.

For that reason I'm 'r-'ing it.
Attachment #374358 - Flags: review?(smichaud) → review-
Comment on attachment 374358 [details] [diff] [review]
fix v1.0

Yes, I know they use it. I'll help them stop using it if this is going to land on mozilla-central.
Attachment #374358 - Flags: review- → review?
Attachment #374358 - Flags: review? → review?(smichaud)
Note that there are some official builds (or at least semi-official builds) of Thunderbird and/or Seamonkey that use mozilla-central.  These builds would be broken by this patch landing on trunk (aka mozilla-central).  See bug 489659.
I know but I'm not going to do anything with those products unless this patch is ready, so we need to review this first. I don't want to do work with those products and then throw it all out or change it if this is a no go for some reason.
Attachment #374358 - Flags: review?(smichaud)
I've already mentioned above (in comment #3) one issue I have with
Josh's patch.  I'll address that myself in a subsequent patch (I hope
I can post it here tomorrow).

I also think nsOSHelperAppService::GetMIMEInfoFromOS() can do a better
job of gleaning MIME information from the OS, so I've rewritten it.

Aside from that, my v2.0 patch is just an update of Josh's patch for
current trunk code.

My revised GetMIMEInfoFromOS() is more careful about checking for a
default application (it uses both the MIME type and the extension to
look for it).  And it takes into account the possibility that
aMIMEType and aFileExt won't match (as happens with bug 488899).

It also sets several additional nsMIMEInfo fields that used to be set
in nsInternetConfigService::FillMIMEInfoForICEntry().

A tryserver build will follow in a few hours.
Attachment #376320 - Flags: review?(joshmoz)
Attachment #376320 - Flags: review?(joshmoz) → review-
Comment on attachment 376320 [details] [diff] [review]
v2.0 patch: More thorough GetMIMEInfoFromOS()

+    CFStringRef CFAppName = NULL;

This will leak if LSCopyItemAttribute sets it.

Fix that and then this looks good.
Attached patch v2.1 patch (fix leak) (obsolete) — Splinter Review
Here's a revision that fixes the leak (and also checks if CFAppName is
NULL).

Thanks for catching the leak, Josh.
Attachment #376320 - Attachment is obsolete: true
Attachment #376461 - Flags: review?(joshmoz)
Attachment #376461 - Flags: superreview?(roc)
Attachment #376461 - Flags: review?(joshmoz)
Attachment #376461 - Flags: review+
Mark, this bug's patch (which removes Internet Config Services) will
break the Thunderbird build that uses mozilla-central (as part of
comm-central), thanks to a mailnews dependency on
nsMIMEInfo::GetMacType() and nsMIMEInfo::GetMacCreator().

But that dependency is very minor, and I think unnecessary.

This patch removes it.
Attachment #376494 - Flags: review?(bugzilla)
Comment on attachment 376461 [details] [diff] [review]
v2.1 patch (fix leak)

+ * Furthermore WebKit has three public methods (in WebKitSystemInterface.h)
+ * that are thin wrappers around this interface's last three methods.

Please file a bug with Apple to get this interface documented. If they don't, we should escalate it.
Attachment #376461 - Flags: superreview?(roc) → superreview+
> Please file a bug with Apple to get this interface documented. If they don't,
> we should escalate it.

I'll get to this as soon as I can (probably later this week).
(In reply to comment #11)
> Created an attachment (id=376494) [details]
> Get rid of mailnews dependency on nsMIMEInfo::GetMacType and GetMacCreator
...
> But that dependency is very minor, and I think unnecessary.
> 
> This patch removes it.

I don't understand what MacType and MacCreator are, this code seems to have been added by bug 122815 which looks like a OS X version 9 bug. Is that why we can now remove this code, because we don't need it for 10.x?

I guess that would make bug 123630 invalid as well?
(In reply to comment #15)

> I don't understand what MacType and MacCreator are

They're Mac-specific, and very old.  OS X still supports them, but
even Apple says extensions should be given precedence over the creator
and/or type (see bug 123630 comment #6).

> this code seems to have been added by bug 122815 which looks like a
> OS X version 9 bug. Is that why we can now remove this code, because
> we don't need it for 10.x?

We shouldn't need this code for OS X.  There may still be subtle
dependencies on an attachment's 'creator' or 'type', in the Mozilla
tree, in other programs (that might load the attachments), or even in
Apple code.  I think that's unlikely ... but we should keep our eyes
open.

The main issue of bug 122815 was fixed by Apple long ago -- OS X
supports file/directory names far longer than 31 characters.

> I guess that would make bug 123630 invalid as well?

I think so (especially in light of bug 123630 comment #6).  But I
can't really follow everything in bug 123630.
Note that my revisions to nsOSHelperAppService::GetMIMEInfoFromOS() should do a better job that previous code of setting an appropriate extension when the original filename doesn't have one.
The patch that just landed for this bug is causing talos crashes and should be backed out or fixed.
Backed out:
http://hg.mozilla.org/mozilla-central/rev/aa3a28d8eb76

I'll make sure tests pass locally before resubmitting a revised version :-(
Attached patch v2.2 patch (fix talos crashes) (obsolete) — Splinter Review
This is work in progress.

Due to test flakiness, I still haven't been able to get through a run of mochitests (with or without the patch).
Duplicate of this bug: 301731
Comment on attachment 376494 [details] [diff] [review]
Get rid of mailnews dependency on nsMIMEInfo::GetMacType and GetMacCreator

Ok, this looks reasonable, I can't see it doing anything in particular.

r=Standard8
Attachment #376494 - Flags: review?(bugzilla) → review+
(In reply to comment #22)

Who should do the superreview?  Do you want to do the honors?
Attached patch fix v2.3 (obsolete) — Splinter Review
Fixes unit test failure. The problem is that the MIME structure is expected to have an empty string for a description when the MIME is bogus/unknown. Mac OS X gives generic descriptions like "Document" when it doesn't recognize the MIME type, so only ask if an application was actually found for the MIME type.
Attachment #374358 - Attachment is obsolete: true
Attachment #376461 - Attachment is obsolete: true
Attachment #376928 - Attachment is obsolete: true
Attachment #376494 - Flags: superreview+
There is another tbox unit test failure holding up this patch, I can't reproduce locally.
I see consistent local failures in the following Mochitests, on two
different machines (one running OS X 10.5.7 and the other running
10.4.11):

  content/media/video/test/test_audio1.html
  content/media/video/test/test_audio2.html
  content/media/video/test/test_bug461281.html
  toolkit/content/tests/widgets/test_videocontrols.html

I'll be trying to uncover the reasons for them.

Josh, are these the failures you saw?
Those aren't the failures I was seeing. I can't find a log with the right failures though.
Attached patch Fix v2.4 (obsolete) — Splinter Review
Here's another revision, which deals with the mochitest failures listed in
comment #27 and an unrelated chrome test failure.

Both sets of failures are partially caused by Apple bugs.  But they can be
easily worked around by making nsOSHelperAppService::GetMIMEInfoFromOS()
stricter -- by making it check more thoroughly for problems, and having it
then set *aFound to PR_FALSE and return only a minimal nsIMIMEInfo structure.

Mochitest Failures from Comment #27

Turns out these are triggered by having RealPlayer installed.  RealPlayer
registers itself with Launch Services to handle the "ogg" extension and the
audio/x-ogg and application/x-ogg MIME types.  But [NSURLFileTypeMappings
MIMETypeForExtension:] returns nil when called with the "ogg" extension (this
is the Apple bug).  In previous code (v2.2 and v2.3) this caused
GetMIMEInfoFromOS() to return an nsIMIMEInfo structure with a null MIME type,
while still setting *aFound to PR_TRUE.  This is what caused all the comment
#27 test failures.

Chrome Test Failure

toolkit/mozapps/downloads/tests/chrome/test_unkownContentType_dialog_layout.xul

On OS X 10.5.7, TextEdit gets registered as the handler for the
application/octet-stream MIME type (indirectly, as a result of being
registered as the handler for the public.data UTI).  Possibly not quite a bug,
but definitely weird.

During the unknownContentType_dialog_layout tests, GetMIMEInfoFromOS() gets
called twice with aFileExt set to "pif" (a deliberately invalid, imaginary
extension).  The first of these calls makes aMIMEType blank; the other sets it
to "application/octet-stream".  Previous code sets *aFound to PR_TRUE on the
second call (since, on OS X 10.5.7, LSGetApplicationForInfo() does tell us
that the application/octet-stream MIME type has a default application).  This
causes failures in two out of the four unknownContentType_dialog_layout tests.

Current code sets *aFound to PR_FALSE even on the second call (since aMIMEType
and aFileExt don't match), and makes all the unknownContentType_dialog_layout
tests pass.
Attachment #377265 - Attachment is obsolete: true
My v2.4 patch passes all tests locally.  But Josh mentions he's seen
some failures (presumably on the tryservers) that he didn't see
locally.

I'm trying to get meaningful data from the tryservers ... but they
take so long (and are so flaky) that this isn't easy.  I'm now waiting
for the results of my current round's "OS X 10.5.2 try hg unit test".
I'll say more when it comes in.
Here's a log of the single legitimate (or seemingly legitimate) test
failure that happens on the tryservers -- in the xpcshell
test_handlerService.js test on the "OS X 10.5.2 try hg unit test"
machine.

I've now seen this failure (on the tryservers) three times over the
last three days.  I don't see it in other people's logs (on the same
machine) from around the same times.  I can't reproduce it locally,
but the log provides enough information that I might be able to figure
out what's going on.
I just realized that this is probably the same failure as Josh posted in comment #24.  Josh thought he'd fixed that.  I guess it must also have another cause.

Josh, I assume this isn't the same failure you mention in comment #26.
(Following up comment #31)

By sheer good luck, I'm able to reproduce this error locally on a partition to which I just installed OS X 10.5.1.  So it shouldn't be too hard to figure out.
Assignee: joshmoz → smichaud
This new revision fixes the failure in test_handlerService.js.

It's also caused by an Apple bug, but that was very easy to work
around.

Turns out the failure has nothing to do with which version of OS X is
installed.  Rather, it happens on systems where the default web
browser has never been explicitly set (programmatically or from the
UI).

On a newly installed copy of OS X, Safari is always the default web
browser.  But this is (presumably) a characteristic of the Launch
Services database copied over by the installer, and not the result of
any explicit settings change.  So (and here's the Apple bug)
LSCopyDefaultHandlerForURLScheme() (in
nsOSHelperAppService::OSProtocolHandlerExists()) doesn't find Safari
when called with aProtocolScheme == "http".  And so
test_handlerService.js fails at line 155
("do_check_false(protoInfo.alwaysAskBeforeHandling);").

I'm waiting for the results of the "OS X 10.5.2 try hg unit test"
tryserver build.  This should come in a few hours ... and hopefully
will confirm that I've fixed the test_handlerService.js failure :-)
Attachment #383983 - Attachment is obsolete: true
The "OS X 10.5.2 try hg unit test" just finished, and the
test_handlerService.js test passed.

(There was a leak, but it was clearly spurious:  The same leak also
happened during the two previous runs of the "OS X 10.5.2 try hg unit
test".)
Attachment #384174 - Flags: review?(joshmoz)
> The "OS X 10.5.2 try hg unit test" just finished, and the
> test_handlerService.js test passed.
>
> (There was a leak, but it was clearly spurious: The same leak also
> happened during the two previous runs of the "OS X 10.5.2 try hg
> unit test".)

Oops, I was looking at the wrong machine.

The "OS X 10.5.2 try hg unit test" finished without any problems at
all!
Comment on attachment 384174 [details] [diff] [review]
Fix v2.5 (fix test_handlerService.js failure)

Thanks for figuring this stuff out!
Attachment #384174 - Flags: review?(joshmoz) → review+
> Thanks for figuring this stuff out!

You're quite welcome.

Does this need sr?  One interface did get changed (nsIMIMEInfo), and
another got dropped altogether (nsIInternetConfigService).
No interfaces have changed since last time this got sr, iirc, so that sr is still valid.
OK, thanks.

I'll land both my patch from comment #11 (suitably updated) and the v2.5 patch.
Comment on attachment 376494 [details] [diff] [review]
Get rid of mailnews dependency on nsMIMEInfo::GetMacType and GetMacCreator

Landed on comm-central:
http://hg.mozilla.org/comm-central/rev/16d18598cb19
Comment on attachment 384174 [details] [diff] [review]
Fix v2.5 (fix test_handlerService.js failure)

Landed on trunk (mozilla-central):
http://hg.mozilla.org/mozilla-central/rev/2816cca3be16
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Improvement: Ts Shutdown decrease 40.96% on Leopard Firefox

Previous results: 288.211 from build 20090623110728 of revision 0cb931e6aaa7 at 2009-06-23 11:07:00 on talos-rev2-leopard13 run # 0

New results: 170.158 from build 20090623121841 of revision 2816cca3be16 at 2009-06-23 12:18:00 on talos-rev2-leopard05 run # 0

Is that for real?
> Improvement: Ts Shutdown decrease 40.96% on Leopard Firefox

> Is that for real?

It wasn't intentional :-)

And there's no obvious reason this patch should make any difference.

Has the change persisted?  Do you see it on more than one machine?

If the answer to both questions is "yes", I'll dig further.  It's
possible I'll uncover some sort of bug (e.g. lots of calls to the
helper app service on shutdown).
I don't know, that was an automated message from catlee's regression finder script:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/9604368e2a2487e1#

Note that that's a claimed 40% *improvement* in shutdown time on Leopard, from 288.211ms to 170.158ms.
> that was an automated message from catlee's regression finder script

I didn't know about that.  Thanks.

> Note that that's a claimed 40% *improvement* in shutdown time on
> Leopard, from 288.211ms to 170.158ms.

Yes.  I wasn't complaining :-)

Such a dramatic change needs explaining, though ... if it's for real.
Ted, your link points to the wrong graph.

But I've found the Ts Shutdown improvement on two Leopard machines
running talos tests on mozilla-central -- talos-rev2-leopard09 (aka
MacOSX Darwin 9.0.0 mozilla-central talos) and talos-rev2-leopard05
(aka MacOSX Darwin 9.0.0 mozilla-central talos nochrome).

Like you, I don't see it on the Tiger machines running talos tests on
mozilla-central.

I'll look further into this when I have time -- maybe later this week.
I wonder if my patch has somehow wrongly been credited with the Ts Shutdown improvement.  The next patch to land (http://hg.mozilla.org/mozilla-central/rev/7a8502b70fdf) seems more likely ... at least to me.
(Following up comment #51)

Though the other patch (changeset 7a8502b70fdf) isn't Mac-specific, which means it's unlikely to have a different effect on Leopard than it does on Tiger.
Duplicate of this bug: 440041
Duplicate of this bug: 436840
(In reply to comment #12)
> (From update of attachment 376461 [details] [diff] [review])
> + * Furthermore WebKit has three public methods (in WebKitSystemInterface.h)
> + * that are thin wrappers around this interface's last three methods.
> 
> Please file a bug with Apple to get this interface documented. If they don't,
> we should escalate it.

(In reply to comment #14)
> > Please file a bug with Apple to get this interface documented. If they don't,
> > we should escalate it.
> 
> I'll get to this as soon as I can (probably later this week).

Did this rdar:// ever get filed?
Blocks: 558552
This almost certainly caused bug 589176.
Depends on: 589176
No longer depends on: 589176
Depends on: 589176
You need to log in before you can comment on or make changes to this bug.