nsHandlerService::remove doesn't remove all assertions

RESOLVED FIXED in mozilla1.9beta1

Status

Core Graveyard
File Handling
P1
normal
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

Trunk
mozilla1.9beta1
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 279561 [details] [diff] [review]
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?
Flags: blocking1.9?
Attachment #279561 - Flags: superreview?(dmose)
Attachment #279561 - Flags: review?(cbiesinger)

Updated

11 years ago
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
Flags: blocking1.9? → blocking1.9+
Attachment #279561 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 1

11 years ago
Created attachment 284707 [details] [diff] [review]
patch v2: unrotted

Updated to apply cleanly now that the fix for bug 393492 has been checked in.
Attachment #279561 - Attachment is obsolete: true
Attachment #284707 - Flags: superreview?(dmose)
Attachment #279561 - Flags: superreview?(dmose)

Comment 2

11 years ago
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.
Attachment #284707 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 3

11 years ago
Created attachment 285021 [details] [diff] [review]
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.
Attachment #284707 - Attachment is obsolete: true
(Assignee)

Comment 4

11 years ago
Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

11 years ago
Status: REOPENED → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 6

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.