517768 - crash with view page source and external editor [@nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) ]

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090919 Minefield/3.7a1pre ID:20090919050513

repro:
open any page
rightclick -> view page source ()

reproducibly: always

External editor:
Notepad 2, http://www.flos-freeware.ch/notepad2.html

this regressed between the 20090917 and 20090918 nightlies
bp-084ed07b-1aaa-4a33-873e-cae282090920
bp-ef749d8f-ab0a-4c79-b7d6-f41ad2090920
bp-f3f39b1f-8b00-4284-9053-7dec52090920

Comment 1

[Boris Zbarsky [:bzbarsky]]

How are you using an external editor here, exactly?  Some particular extension?

What are the hours on those nightlies (or better yet the changeset IDs from about:buildconfig)?

Comment 2

[Peter van der Woude [:Peter6]]

(In reply to comment #1)
> How are you using an external editor here, exactly?  Some particular extension?
> 
> What are the hours on those nightlies (or better yet the changeset IDs from
> about:buildconfig)?

1. via about:config
view_source.editor.args; no value
view_source.editor.external;true
view_source.editor.path;C:\Program Files\Notepad2\notepad2.exe
(meaning i didn't use an extension)

2.
the last changeset included in the build of the 17th is 3079370d6597
the last changeset included in the build of the 18th is 41dd493c42c9

Comment 3

[Boris Zbarsky [:bzbarsky]]

So this pushlog:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3079370d6597&tochange=41dd493c42c9

There was in fact a change to OnStateChange in there (bug 517272), but I wouldn't expect that to cause crashes...  That said, that's the code where the crash is claimed to happen according to breakpad.  Line 5587, as listed in those crash reports in comment 0, is:

5586 if (NS_SUCCEEDED(mPrefs->GetBoolPref("ui.use_activity_cursor", &tmpBool))
5587 && tmpBool) { 

Ted, what's the chance that the line number is .... misleading?

Peter, does the crash depend on the exact app you use for editor.path?

Comment 4

[Peter van der Woude [:Peter6]]

Boris,

nope, I changed the app to Notepad (C:\WINDOWS\system32\notepad.exe) and Wordpad (C:\WINDOWS\system32\write.exe) which come with Windows and crash with those 2 too.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  I can reproduce this (on mac).  mPrefs is null.  This is a regression from bug 482985.

Comment 6

[Markus Stange [:mstange]]

So OnStateChange can be called before Create? Interesting.
Boris, want me to add a null check?

Comment 7

[Boris Zbarsky [:bzbarsky]]

And mPrefs is null because someone "created" a docshell without actually calling Create() on it.  The someone is openInExternalEditor in toolkit/components/viewsource/content/viewSourceUtils.js, which has this excellent hunk of code:

138           // we'll use nsIWebPageDescriptor to get the source because it may
139           // not have to refetch the file from the server
140           // XXXbz this is so broken...  This code doesn't set up this docshell
141           // at all correctly; if somehow the view-source stuff managed to
142           // execute script we'd be in big trouble here, I suspect.
143           var webShell = Components.classes["@mozilla.org/docshell;1"].createInstance();
144           this.viewSourceProgressListener.webShell = webShell;
145           var progress = webShell.QueryInterface(this.mnsIWebProgress);
146           progress.addProgressListener(this.viewSourceProgressListener,
147                                        this.mnsIWebProgress.NOTIFY_STATE_DOCUMENT);
148           var pageLoader = webShell.QueryInterface(this.mnsIWebPageDescriptor);    
149           pageLoader.loadPage(aPageDescriptor, this.mnsIWebPageDescriptor.DISPLAY_AS_SOURCE);

With the change from bug 482985, we now actually rely on the docshell being "set up correctly" (at least insofar as basic stuff like Create() being called goes).  We could either stop relying on that (by adding a null-check for mPrefs), or fix the view-source code in toolkit.... preferably the latter.  Feel free to punt back if the former is more desirable for some reason.

Comment 8

[Peter van der Woude [:Peter6]]

nb:
when I view the source of a local file the crash does not occur

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to comment #3)
> Ted, what's the chance that the line number is .... misleading?

Breakpad just maps instructions to line numbers based on whatever is in the debug info, so this must be what the compiler reports. Looking at the symbol dump, I don't see line 5586 listed at all there.

Comment 10

[Dão Gottwald [:dao]]

Attachment: fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 404240 [details] [diff] [review]
fix

I believe this leaks the docshell for the lifetime of the app, since Create() makes the docshell hold a ref to the prefservice (via mPrefs) and vice versa (via the "browser.xul.error_pages.enabled" pref observer).

I _think_ it should be safe to just call destroy() in the viewSourceProgressListener.destroy function before nulling out its webShell.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Call destroy() on the docshell, that is.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 404294 [details] [diff] [review]
patch v2

you don't need the nsIDocShell QI; other than that, looks good.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/72eb60dd0f50

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/12a923071ba8