394838 - nsHandlerService::remove doesn't remove all assertions

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problems

nsHandlerService::remove doesn't remove all assertions about a handler info.  It misses multiple assertions with the same property for a given source (f.e. multiple NC:possibleApplication assertions for a handler info resource), and it doesn't remove assertions about possible apps (f.e. the NC:executable and NC:uriTemplate assertions).

It should remove both kinds of assertions, so that when you remove a handler entry using the available UI, all information about that entry is removed from the datastore.  But it should not remove assertions about a possible app that is still being used by another handler info.

The problems are mitigated somewhat by the fact that it does successfully remove the assertion that determines whether or not the handler info is considered to be registered in the datasource, so Firefox won't think the handler info is registered once it has been removed.  But we still shouldn't be leaving cruft in the datasource.

Here's a patch that fixes both problems.  Note that it conflicts trivially with the patch on bug 393492, which is currently awaiting checkin approval.  Not sure who to ask for review.  Is biesi still available for such things?

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: unrotted

Updated to apply cleanly now that the fix for bug 393492 has been checked in.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 284707 [details] [diff] [review]
patch v2: unrotted


>+    while (possibleHandlerTargets.hasMoreElements()) {
>+      let possibleHandlerTarget = possibleHandlerTargets.getNext();
>+      if (possibleHandlerTarget instanceof Ci.nsIRDFResource)

Can you add a comment that the if here is to deal with possibly corrupt RDF files?

sr=dmose with that change.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: addresses superreview issue

(In reply to comment #2)
> Can you add a comment that the if here is to deal with possibly corrupt RDF
> files?

Yup, here's a version that says:

      // Note: possibleHandlerTarget should always be an nsIRDFResource.
      // The conditional is just here in case of a corrupt RDF datasource.

This is the version I'll check in.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.13; previous revision: 1.12
done

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I backed this out as part of the process of tracking down the Ts/Tp regressions (bug 399955, bug 400045).  Will check back in once those are under control.

Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.14; previous revision: 1.13
done

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Relanded per Sheriff Dietrich.

Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.15; previous revision: 1.14
done