Closed Bug 788741 Opened 7 years ago Closed 7 years ago

Crash in nsWebBrowserPersist::CalculateAndAppendFileExt() with too long file extension

Categories

(Core Graveyard :: Embedding: APIs, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: ttaubert, Assigned: drexler)

References

()

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(1 file, 2 obsolete files)

I can easily crash Firefox by opening http://hbr.org/2012/09/too-many-pivots-too-little-passion/ar/1 and then trying to save it locally as "Web Page, complete". It fails with the following error:

###!!! ASSERTION: Truncate cannot make string longer: 'newLength <= mLength', file ../../dist/include/nsTSubstring.h, line 494
###!!! ABORT: OOM: file /home/tim/workspace/fx-team/xpcom/string/src/nsTSubstring.cpp, line 533

--------------

The problem here is that the URL

http://hbr.org.woopra-ns.com/visit/ra=0JC9SNP0FDQXBK2ST4VOZNAQMDC9FBF6&alias=hbr.org&cookie=&meta=NDgyMjgzNDMmNyY3JjAmMTM0Njg2MDA1MTY0NyYxMzQ2ODcwNDk4NTQwJiYxMDAmJjImJiYm&screen=1366x768&language=en-US&referer=&idle=0&vs=r&ce_type=pageview&ce_title=Too%20Many%20Pivots%2C%20Too%20Little%20Passion%0A%09%09%09%09%20-%20Harvard%20Business%20Review&ce_url=%2F2012%2F09%2Ftoo-many-pivots-too-little-passion%2Far%2F1&ce_name=pv

produces a fileName "ra0JC9SNP0FDQXBK2ST4VOZNAQMDC9FBF6aliashbr" and an extension containing the rest of the URL. The code here tries to truncate the name the file will be saved under to fit into the limit of kDefaultMaxFilenameLength. It doesn't take a too long extension into account.

I wasn't sure what approach should be chosen to fix this. Should we truncate the extension? Should we move as many characters as possible from the extension to the file name? Should we not allow extensions this long at all?
On Windows: bp-f95096c0-3352-41f2-bf8e-5a1ac2120905.
Severity: normal → critical
Crash Signature: [@ mozalloc_abort | NS_DebugBreak_P | nsACString_internal::SetLength ] → [@ mozalloc_abort | NS_DebugBreak_P | nsACString_internal::SetLength ] [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsACString_internal::SetCapacity(unsigned int)]
Keywords: reproducible
I think it's safe to truncate the extension to if it's long enough...
Duplicate of this bug: 789869
Assignee: nobody → andrew.quartey
Attached patch patch (obsolete) — Splinter Review
Truncates the file name and/or extension if either is longer than the default file name length.
Attachment #661442 - Flags: review?(bzbarsky)
I think we still need to check if |(newFileName.Length() + fileExt.Length() + 1) > kDefaultMaxFilenameLength|. Both parts can be smaller but concatenated they're larger than kDefaultMaxFilenameLength.

Also, Truncate() does probably not do the right thing if you use |newLength| to truncate the whole filename.
Comment on attachment 661442 [details] [diff] [review]
patch


Vacating review request as patch is erroneous.

> I think we still need to check if |(newFileName.Length() + fileExt.Length()
> + 1) > kDefaultMaxFilenameLength|. Both parts can be smaller but
> concatenated they're larger than kDefaultMaxFilenameLength.

True. I didn't think about that case. 

> Also, Truncate() does probably not do the right thing if you use |newLength|
> to truncate the whole filename.

In the case where the filename and extension are both longer than the |kDefaultMaxFilenameLength|, what is given priority in fitting the desired length - most of the filename or extension?
Attachment #661442 - Flags: review?(bzbarsky)
What I would recommend is that if the whole name is too long then if extension is > kDefaultMaxFilenameLength/2 then you truncate the extension to kDefaultMaxFilenameLength/2.  After that, truncate the filename as needed.
Attached patch patch (obsolete) — Splinter Review
Attachment #661442 - Attachment is obsolete: true
Attachment #662209 - Flags: review?(bzbarsky)
Comment on attachment 662209 [details] [diff] [review]
patch

This will still truncate to negative if fileExt is long but newFileName is not, won't it?
Attachment #662209 - Flags: review?(bzbarsky) → review-
Attached patch patchSplinter Review
(In reply to Boris Zbarsky (:bz) from comment #9)

> This will still truncate to negative if fileExt is long but newFileName is
> not, won't it?

Oops. It would.
Attachment #662209 - Attachment is obsolete: true
Attachment #662258 - Flags: review?(bzbarsky)
Comment on attachment 662258 [details] [diff] [review]
patch

This, I buy.
Attachment #662258 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e1850b0b8d4a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.