Closed Bug 435881 Opened 11 years ago Closed 2 years ago

Move mailnews/compose to frozen linkage

Categories

(MailNews Core :: Composition, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

Another step in the move to frozen linkage. Patches coming soon.
Target Milestone: --- → mozilla1.9
These are simple changes (mainly includes) to files that don't need (m)any other changes.
Attachment #322661 - Flags: superreview?(bienvenu)
Attachment #322661 - Flags: review?(bienvenu)
Attachment #322661 - Flags: superreview?(bienvenu)
Attachment #322661 - Flags: superreview+
Attachment #322661 - Flags: review?(bienvenu)
Attachment #322661 - Flags: review+
Attachment #322661 - Attachment description: Simple Changes → [checked in] Simple Changes
I think this bug introduced a regression - nsMsgCompFields::SplitRecipients crashes if a null recipient str is passed in, though first there is an assertion to fix the callers. This patch attempts to do that. The crash is in the nsDependentString(recipientsStr) piece of code.

STR - have an empty To: address, and switch the from identity a couple times...
Attachment #322944 - Flags: review?(bugzilla)
Comment on attachment 322944 [details] [diff] [review]
[checked in]fix callers of nsMsgCompFields::SplitRecipients

I'm giving a provisional r+. I think we should fix the SplitRecipients as well to do the right thing as it is an idl method and extensions can call it, so we shouldn't crash if they do something wrong.
Attachment #322944 - Flags: review?(bugzilla) → review+
I agree - that was what I did at first, but then I read the assertion and went and fixed the callers. I'll attach my patch to fix SplitRecipients as well.
Attached patch bulletproof SplitRecipients (obsolete) — Splinter Review
trivial check
Attachment #322949 - Flags: superreview?(neil)
Attachment #322949 - Flags: review?(bugzilla)
Attachment #322949 - Flags: review?(bugzilla) → review+
Comment on attachment 322661 [details] [diff] [review]
[checked in] Simple Changes

>@@ -668,7 +668,7 @@ nsresult nsMsgCompFields::SplitRecipients
>-  CopyUTF16toUTF8(recipients, recipientsStr);
>+  CopyUTF16toUTF8(nsDependentString(recipients), recipientsStr);
>   rv = parser->ParseHeaderAddresses("UTF-8", recipientsStr.get(), &names,
>                                     &addresses, &numAddresses);
Why not make recipients an AString or AUTF8String?
Duplicate of this bug: 436544
Attachment #322944 - Attachment description: fix callers of nsMsgCompFields::SplitRecipients → [checked in]fix callers of nsMsgCompFields::SplitRecipients
> Why not make recipients an AString or AUTF8String?
> 

sorry, didn't realize that was for me. I can do that, AString, I think, would be a more straightforward change
Comment on attachment 322949 [details] [diff] [review]
bulletproof SplitRecipients

As per comment #6.
Attachment #322949 - Flags: superreview?(neil) → superreview-
this also contains an unrelated change in my tree, to use const with an nsACString that someone suggested in a drive by comment, iirc.
Attachment #325037 - Flags: superreview?(neil)
Attachment #325037 - Flags: superreview?(neil) → superreview+
Attachment #325037 - Attachment description: patch addressing comments → [checked in]patch addressing comments
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Although the patch just checked in fixes the crash it doesn't fix the bug, we've got more work to do on this yet, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P3
Product: Core → MailNews Core
Assignee: bugzilla → nobody
Status: REOPENED → NEW
Priority: P3 → --
Target Milestone: mozilla1.9 → ---
Attachment #322949 - Attachment is obsolete: true
Blocks: SM-OOPP
No longer blocks: SM-OOPP
Blocks: mailnews-libxul
No longer blocks: 377319
Serge, why are you changing around dependencies here like this would have been fixed when it hasn't? That doesn't block building with a libxul variant as we already do this, but we're still not building with frozen linkage and it's not at all clear that doing that works in any way, AFAIK.
(In reply to comment #12)

I did that because bug 377319 is 'fixed' and bug 432162 (which is block by bug 377319) is (still) 'open'.
But maybe some bug statuses are wrong (now), iiuc your comment :-|
I believe this bug should be reresolved, unless anyone else knows different?
(In reply to neil@parkwaycc.co.uk from comment #14)
> I believe this bug should be reresolved, unless anyone else knows different?

Fixed according to Neil
Status: NEW → RESOLVED
Closed: 11 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.