1308327 - Crash in nsDependentString::nsDependentString called from nsIEHistoryEnumerator::HasMoreElements()

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-b1dfb174-89ce-4706-ba04-7a9fe2161005.
=============================================================

This is the number 9 crash on Windows for the Oct 5 Nightly, with 12 crashes. These are null deref crashes, from different install times. (I'm adding nsDependentString to the prefix list in bug 1308320 to hopefully split these signatures out better going forward.)

Looking at SuperSearch, it appears that the nsIEHistoryEnumerator variant first appeared in the 10-4 Nightly, so I think this could be a regression from bug 1302855: 
https://crash-stats.mozilla.com/api/SuperSearch/?signature=%3DnsDependentString%3A%3AnsDependentString&proto_signature=~nsIEHistoryEnumerator&product=Firefox

Comment 1

[Andrew McCreight [:mccr8]]

Enes, do you have time to look into this? Or have some idea what might be going wrong? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

I guess this is actually the link I need for my search results: https://crash-stats.mozilla.com/search/?signature=%3DnsDependentString%3A%3AnsDependentString&proto_signature=~nsIEHistoryEnumerator&product=Firefox&_sort=-date&_facets=build_id&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-build_id

Comment 3

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: Crashes in IE migration tool. It seems like a bad time to crash when somebody is considering using Firefox.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Use a string class that is okay with null values

nsDependentSubstring will handle null values correctly,
while nsDependentString will assert.

This should fix the crash, I don't have an explanation as to why we weren't
seeing it before. I was going to have :jaws review this, but they have reviews
blocked. I'm not sure of a more appropriate reviewer.

MozReview-Commit-ID: CZYdmsVisrc

Comment 5

[Enes Göktaş [:egoktas]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Enes, do you have time to look into this? Or have some idea what might be
> going wrong? Thanks.

Hm, not sure why this is caused after the browsercomps fold into xul. We didn't really change the logic when merging these libraries. Maybe Benjamin can tell.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8798716 [details] [diff] [review]
Use a string class that is okay with null values

Review of attachment 8798716 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/nsIEHistoryEnumerator.cpp
@@ +79,5 @@
>        return HasMoreElements(_retval);
>      }
>    }
>  
> +  nsDependentSubstring title(statURL.pwcsTitle);

Makes sense, pwcsTitle is a LPWSTR which MAY be null-terminated, or not. nsDependentString requires the string to always be null-terminated so it was not suitable. it's unfortunate we don't have data len, since this constructor otherwise may create a UINT32_MAX len buffer. I couldn't find any documentation whether pwcsTitle is always null or null terminated, but since the API doesn't pass out a len, I guess we could assume it is and use wcslen? What do you think?

Comment 7

[Marcia Knous [:marcia]]

Tracking 52+ for this crash for all the reasons in Comment 3.

Comment 9

[Kamil Jozwiak [:kjozwiak]]

STR Used:

* install the latest version of m-c, leaving everything as default
* once the installation has completed, select "Finish" with "Launch Nightly now" selected
* under the "Import Settings and Data", ensure "Internet Explorer" is selected, click "Next"

You'll get an instant crash every time.

Crash Reports:

* https://crash-stats.mozilla.com/report/index/f3959012-76a1-40c7-86c8-8311a2161018
* https://crash-stats.mozilla.com/report/index/25c74ea9-e1fd-4977-82bd-38a7c2161018

Comment 10

[Andrew McCreight [:mccr8]]

Is this ready to land, Eric?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #9)
> You'll get an instant crash every time.

Now that there's STR it would be good if somebody checked to ensure that older branches are not affected by this. It isn't really clear from the patch why only 52 started being affected.

Comment 12

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to Kamil Jozwiak [:kjozwiak] from comment #9)
> > You'll get an instant crash every time.
> 
> Now that there's STR it would be good if somebody checked to ensure that
> older branches are not affected by this. It isn't really clear from the
> patch why only 52 started being affected.

I thought this was a regression from bug 1302855 ? I kind of assumed this was all public vs. private string API malarkey (which I understand is not really an explanation without more detail). browsercomps had to use the external string API, and now it doesn't because it's linked into libxul.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to Kamil Jozwiak [:kjozwiak] from comment #9)
> > You'll get an instant crash every time.
> 
> Now that there's STR it would be good if somebody checked to ensure that
> older branches are not affected by this. It isn't really clear from the
> patch why only 52 started being affected.

I could only reproduce the crash using fx52.0a1. For each case listed below, I went through the STR in comment#9 five different times.

* Win 7 x64 using fx52.0a1 - reproduced several times
* Win 7 x64 using fx51.0a2 - couldn't reproduce
* Win 7 x64 using fx50.0b8 - couldn't reproduce
** used both "Firefox Start, a fast home page with built-in search" and "Import your home page from IE" under "Home Page Selection"
* Win 7 x64 using fx49.0.1 - couldn't reproduce
** used both "Firefox Start, a fast home page with built-in search" and "Import your home page from IE" under "Home Page Selection"

Used the following builds:
* https://archive.mozilla.org/pub/firefox/nightly/2016/10/2016-10-18-03-02-11-mozilla-central/
* https://archive.mozilla.org/pub/firefox/nightly/2016/10/2016-10-18-00-40-15-mozilla-aurora/
* https://archive.mozilla.org/pub/firefox/candidates/50.0b8-candidates/build1/win32/en-US/
* https://archive.mozilla.org/pub/firefox/releases/49.0.1/win32/en-US/

Comment 14

[Andrew McCreight [:mccr8]]

Great, thanks for confirming that it doesn't affect older branches! It did seem like some weird public private string API key issue, as Gijs said, but I don't think anybody analyzed the issue enough to figure out if that was really the cause, rather than say some cluster of IE users on Nightly doing something odd.

Comment 15

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e346404336

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8798716 [details] [diff] [review]
Use a string class that is okay with null values

Review of attachment 8798716 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/nsIEHistoryEnumerator.cpp
@@ +79,5 @@
>        return HasMoreElements(_retval);
>      }
>    }
>  
> +  nsDependentSubstring title(statURL.pwcsTitle);

Yes I think it's safe to assume it's null terminated if a length is not provided.

As it turns out nsDependentSubstring doesn't have the right constructor for a wchar_t (I think). I'll try a slightly different approach.

Comment 17

[Eric Rahm [:erahm]]

Attachment: Avoid passing a null value to nsDependentString

The internal version of nsDependentString will assert if a null value is
passed in. An empty string should be okay though.

MozReview-Commit-ID: CZYdmsVisrc

Comment 18

[Eric Rahm [:erahm]]

(clearing ni, latest patch still needs review)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a2511b2c5a
Avoid passing a null value to nsDependentString. r=mak

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25a2511b2c5a

Comment 21

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/10/2016-10-24-03-02-05-mozilla-central/

Test Cases Used:

* original STR from comment#9
* ensured fx launches without any issues when selecting "Don't import anything"
* ensured fx launches without any issues when selecting selecting "Cancel" under the "Import Wizard"

Platforms Used:

* Win 7 SP1 x64 VM - PASSED (x5 for each build)
** using fx52.0a1, buildId: 20161024030205, changeset: 215f96861176 (x86 & x64)

* Win 7 SP1 x86 VM - PASSED (x5 for each build)
** using fx52.0a1, buildId: 20161024030205, changeset: 215f96861176 (x86)

Florin, when SV starts going through their smoke tests, could you double check and make sure that importing data from other browsers into fx hasn't regressed under Win 7? I went through some basic tests and it seems like everything is working. I'm not sure if Win 7 is a platform that SV checks when they run through their import/export smoke tests.

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Win 7 is covered often during the Beta cycle but not necessarily for Migration. Will add Andrei and Cornel here, so they can keep the ni? for tracking until we get to test Migration in Beta 52, and make sure we cover Win 7 as one of our 4 platforms.

Comment 23

[Ciprian Georgiu, Desktop QA]

We have also verified this issue during our Beta tests around Migration. We can confirm this fix on 52.0b4 (20170206101855) under Windows 7 x64 and Windows 10 x86.