422722 - crash when setting frame src in new webshell instance [@ nsDocShell::DoChannelLoad]

Status: RESOLVED INCOMPLETE

(Core :: DOM: Navigation, defect)

Description

[Bret]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1) ; SLCC1; .NET CLR 2.0.50727; .NET CLR 3.0.04506; .NET CLR 1.1.4322; eMusic DLM/4; Zune 2.0)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

Firefox crashes when an extension loads a page in a new webshell instance and attempts to set the src attribute of a frame within the loaded page. (Minimal extension to reproduce the problem will be attached).

The same works fine in Firefox 1.5 and 2.0.

Reproducible: Always

Steps to Reproduce:
1. Install the attached Firefox extension
2. Open a page with frames (such as frames.html attached)
3. Click on the doit button
Actual Results:  
Firefox crashes (exits immediatly)

Expected Results:  
Firefox does not crash.
The frame src is updated correctly.

Comment 1

[Bret]

Attachment: extension used to reproduce the problem

Comment 2

[Bret]

Attachment: example page with frames to reproduce the problem

1. Unzip the attachment
2 [review]. Open the frames.html file in Firefox. 
3. Click on the doit button of the extension. 
=> Crash

Comment 3

[Bret]

Attachment: extension used to reproduce the problem

sorry, I just realized that my first attachment had some unecessary code.

Comment 4

[timeless]

you should be able to attach a stack trace, either by following the instructions on: http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg
or by triggering breakpad about loading about:crashes (just copy the id here and we can go from there).

Comment 5

[Bret]

Here it is:
http://crash-stats.mozilla.com/report/index/dade1ca3-f1c7-11dc-a93a-001a4bd46e84

Comment 6

[Bret]

And from WinDbg:
(114.5ec): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00000000 ebx=00200001 ecx=00000000 edx=00000000 esi=01fe0cfc edi=00d6f7c0
eip=605e9216 esp=0012ede0 ebp=0012edf0 iopl=0         nv up ei pl zr na pe cy
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010247
xul!nsDocShell::DoChannelLoad+0x43:
605e9216 8b08            mov     ecx,dword ptr [eax]  ds:0023:00000000=????????
0:000> kp
ChildEBP RetAddr  
0012edf0 605113d6 xul!nsDocShell::DoChannelLoad(class nsIChannel * aChannel = 0x01fe0cfc, class nsIURILoader * aURILoader = 0x01347f40, int aBypassClassifier = 0)+0x43 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 7461]
0012ee50 604fdb47 xul!nsDocShell::DoURILoad(class nsIURI * aURI = 0x025c5be0, class nsIURI * aReferrerURI = 0x025c59d0, int aSendReferrer = 1, class nsISupports * aOwner = 0x02912330, char * aTypeHint = 0x00000000 "", class nsIInputStream * aPostData = 0x00000000, class nsIInputStream * aHeadersData = 0x00000000, int aFirstParty = 0, class nsIDocShell ** aDocShell = 0x00000000, class nsIRequest ** aRequest = 0x01fe0d08, int aIsNewWindowTarget = 0, int aBypassClassifier = 0)+0x21a [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 7333]
0012eff0 6050acea xul!nsDocShell::InternalLoad(class nsIURI * aURI = 0x025c5be0, class nsIURI * aReferrer = 0x025c59d0, class nsISupports * aOwner = 0x02912330, unsigned int aFlags = 0, wchar_t * aWindowTarget = 0x0032f25a "", char * aTypeHint = 0x00000000 "", class nsIInputStream * aPostData = 0x00000000, class nsIInputStream * aHeadersData = 0x00000000, unsigned int aLoadType = 1, class nsISHEntry * aSHEntry = 0x00000000, int aFirstParty = 0, class nsIDocShell ** aDocShell = 0x00000000, class nsIRequest ** aRequest = 0x00000000)+0x494 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 7066]
0012f088 6060dbad xul!nsDocShell::LoadURI(class nsIURI * aURI = 0x0012efc8, class nsIDocShellLoadInfo * aLoadInfo = 0x600cdfa2, unsigned int aLoadFlags = 0x10, int aFirstParty = 1611444937)+0x24a [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp @ 903]
0012f0b4 6060dca6 xul!nsFrameLoader::LoadURI(class nsIURI * aURI = 0x025c5be0)+0xdd [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\base\src\nsframeloader.cpp @ 230]
0012f180 609456c1 xul!nsFrameLoader::LoadFrame(void)+0xda [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\base\src\nsframeloader.cpp @ 157]
0012f18c 60982033 xul!nsGenericHTMLFrameElement::LoadSrc(void)+0x1c [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nsgenerichtmlelement.cpp @ 2977]
0012f1a0 609836ee xul!nsGenericHTMLFrameElement::SetAttr(int aNameSpaceID = 0, class nsIAtom * aName = 0x0036191c, class nsIAtom * aPrefix = 0x00000000, class nsAString_internal * aValue = 0x0012f470, int aNotify = 1)+0x34 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nsgenerichtmlelement.cpp @ 3033]
0012f1b8 60983ec4 xul!nsGenericHTMLElement::SetAttr(int aNameSpaceID = 1242068, class nsIAtom * aName = 0x02946664, class nsAString_internal * aValue = 0x00000045, int aNotify = 1)+0x1c [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nsgenerichtmlelement.h @ 207]
0012f1cc 60984bc1 xul!nsGenericHTMLElement::SetAttrHelper(class nsIAtom * aAttr = 0x02946664, class nsAString_internal * aValue = 0x00000045)+0x13 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nsgenerichtmlelement.cpp @ 2288]
0012f1dc 6065b693 xul!nsHTMLFrameElement::SetSrc(class nsAString_internal * aValue = 0x02946664)+0x17 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nshtmlframeelement.cpp @ 113]
0012f1f0 60596375 xul!NS_InvokeByIndex_P(class nsISupports * that = 0x02946664, unsigned int methodIndex = 0x45, unsigned int paramCount = 1, struct nsXPTCVariant * params = 0x0012f2a0)+0x27 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 102]
0012f260 60a03969 xul!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x00000000, XPCWrappedNative::CallMode mode = 43279972 (No matching enumerant))+0x525 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2369]
00000000 00000000 xul!nsHTMLFrameElement::QueryInterface(struct nsID * aIID = <Memory access error>, void ** aInstancePtr = <Memory access error>)+0x34 [e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\html\content\src\nshtmlframeelement.cpp @ 98]

Comment 7

[timeless]

Attachment: don't rely on mPrefs

Comment 8

[Boris Zbarsky [:bzbarsky]]

Uh... no.  This code is just misusing docshell.  It won't work right, and I don't think we should be trying to make it work right.

I agree that it shouldn't crash. I think it should throw, probably from the loadPage call.

Comment 9

[timeless]

that would mean effectively guarding virtually all docshell/webshell methods with an mPrefs/mCreated check.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm happy just crashing if it's misused too, to be honest.  Or we could remove that contract, which might be kind of nice too.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 309813 [details] [diff] [review]
don't rely on mPrefs

what bz said

Comment 12

[timeless]

toolkit/components/viewsource/content/viewSourceUtils.js:
          // we'll use nsIWebPageDescriptor to get the source because it may
          // not have to refetch the file from the server
          // XXXbz this is so broken...  This code doesn't set up this docshell
          // at all correctly; if somehow the view-source stuff managed to
          // execute script we'd be in big trouble here, I suspect.
          var webShell = Components.classes["@mozilla.org/docshell;1"].createInstance();
          webShell.QueryInterface(Components.interfaces.nsIBaseWindow).create();

So, what's the correct way to replace this?

Comment 13

[timeless]

Attachment: untested

i believe the c++ changes would compile (i haven't spoken to a compiler, my tree needs a rebuild).

the js changes are entirely untested...

bz: would the js work or is this going to shoot some other object in some interesting way?

i expect it needs to unregister its listener...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496460 [details] [diff] [review]
untested

This looks like it would break the world.   And the js change makes no sense.

Comment 15

[timeless]

bz: so err. how would one do something such that the js could be remotely happy?

Comment 16

[Boris Zbarsky [:bzbarsky]]

It could set up the load itself, the way docshell does in loadPage.

Comment 17

[timeless]

Attachment: c++ bits compiling

sorry, i'm confused.

@@ -130,25 +130,21 @@ var gViewSourceUtils = {
           webBrowserPersist.progressListener = this.viewSourceProgressListener;
           webBrowserPersist.saveURI(uri, null, null, null, null, file);
 
           // register the file to be deleted on app exit
           Components.classes["@mozilla.org/uriloader/external-helper-app-service;1"]
                     .getService(Components.interfaces.nsPIExternalAppLauncher)
                     .deleteTemporaryFileOnExit(file);
         } else {
           // we'll use nsIWebPageDescriptor to get the source because it may
           // not have to refetch the file from the server
-          // XXXbz this is so broken...  This code doesn't set up this docshell
-          // at all correctly; if somehow the view-source stuff managed to
-          // execute script we'd be in big trouble here, I suspect.
-          var webShell = Components.classes["@mozilla.org/docshell;1"].createInstance();
-          webShell.QueryInterface(Components.interfaces.nsIBaseWindow).create();
+          var webShell = getWebNavigation();
           this.viewSourceProgressListener.webShell = webShell;
           var progress = webShell.QueryInterface(this.mnsIWebProgress);
           progress.addProgressListener(this.viewSourceProgressListener,
                                        this.mnsIWebProgress.NOTIFY_STATE_DOCUMENT);
           var pageLoader = webShell.QueryInterface(this.mnsIWebPageDescriptor);    
           pageLoader.loadPage(aPageDescriptor, this.mnsIWebPageDescriptor.DISPLAY_AS_SOURCE);
         }
       }
     } catch (ex) {
       // we failed loading it with the external editor.

webShell isa docShell, and it's calling LoadPage()

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 497127 [details] [diff] [review]
c++ bits compiling

The JS change still doesn't make sense.  And this still breaks the world.  Now can you please stop bothering me with this?

Comment 19

[Andrei Purice]

Hey timeless,
Should we consider this issue closed since the addon in the bug isn't working anymore but the crashes no longer occur on the latest versions of Firefox Nightly 91.0a1, beta 90.0b8 or release 89.0.1 using the provided test cases on Windows 10.

Comment 20

[Andrew McCreight [:mccr8]]

I think it is fine to close this. It looked like the issue was something about how we expose docshell to JS because of some interaction with a XUL addon, and XUL addons are no longer supported.