Closed Bug 683777 Opened 12 years ago Closed 12 years ago

Firefox Crash in nsDocShell::InternalLoad

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Version:
8 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla10
Tracking Flags:
Tracking Status
firefox8 + fixed
firefox9 + fixed
firefox10 --- fixed

People

(Reporter: marcia, Assigned: justin.lebar+bug)

References

Details

(Keywords: crash, regression, Whiteboard: [inbound][qa-])

Crash Data

Attachments

(3 files)

Backout part 2, v1
84.04 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Backout part 1, v1
37.89 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Patch for branches
1.30 KB, patch
christian
: approval-mozilla-aurora+
christian
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Seen while reviewing crash stats and the explosive report. Seen across versions 7, 8 and 9. https://crash-stats.mozilla.com/report/list?signature=nsDocShell%3A%3AInternalLoad%28nsIURI%2A%2C%20nsIURI%2A%2C%20nsISupports%2A%2C%20unsigned%20int%2C%20wchar_t%20const%2A%2C%20char%20const%2A%2C%20nsIInputStream%2A%2C%20nsIInputStream%2A%2C%20unsigned%20int%2C%20nsISHEntry%2A%2C%20int%2C%20nsIDocShell%2A%2A%2C%20nsIRequest%2A%2A%29 to the report swhich are all Windows.

https://crash-stats.mozilla.com/report/index/7f591dd6-a803-4015-a49f-958ae2110825 

Frame 	Module 	Signature [Expand] 	Source
0 		@0x10244c8b 	
1 	xul.dll 	nsDocShell::InternalLoad 	docshell/base/nsDocShell.cpp:8330
2 	xul.dll 	nsDocShell::LoadURI 	docshell/base/nsDocShell.cpp:1471
3 	xul.dll 	nsFrameLoader::ReallyStartLoadingInternal 	content/base/src/nsFrameLoader.cpp:487
4 	xul.dll 	nsFrameLoader::ReallyStartLoading 	content/base/src/nsFrameLoader.cpp:428
5 	xul.dll 	nsDocument::MaybeInitializeFinalizeFrameLoaders 	
6 	xul.dll 	nsDocument::EndUpdate 	content/base/src/nsDocument.cpp:3990
7 	xul.dll 	nsHTMLDocument::EndUpdate 	content/html/document/src/nsHTMLDocument.cpp:2523
8 	xul.dll 	nsINode::ReplaceOrInsertBefore 	content/base/src/nsGenericElement.cpp:4038
9 	xul.dll 	nsIDOMNode_AppendChild 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:6863
10 	mozjs.dll 	CallCompiler::generateNativeStub 	js/src/methodjit/MonoIC.cpp:813
11 	mozjs.dll 	js::mjit::ic::NativeCall 	js/src/methodjit/MonoIC.cpp:1031
12 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:686
13 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:733
14 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:4128
15 	mozjs.dll 	UncachedInlineCall 	js/src/methodjit/InvokeHelpers.cpp:320
16 	mozjs.dll 	js::mjit::stubs::UncachedCallHelper 	js/src/methodjit/InvokeHelpers.cpp:418
17 	mozjs.dll 	CallCompiler::update 	js/src/methodjit/MonoIC.cpp:963
18 	mozjs.dll 	js::mjit::ic::Call 	js/src/methodjit/MonoIC.cpp:1017
19 	mozjs.dll 	JSCompartment::wrap 	js/src/jscompartment.cpp:363
20 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:686
21 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:733
22 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:610
23 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:686
24 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:805
25 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5070
26 	xul.dll 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1657
27 	xul.dll 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:585
28 	xul.dll 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:114
29 	xul.dll 	SharedStub 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:141
30 	xul.dll 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1083
Also seen on Windows XP. Observed today on 10.0a1 on a web site I've been using every day with Nightly builds.

User Agent: Mozilla/5.0 (Windows NT 5.2; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1
(In reply to B.J. Herbison from comment #1)
> on a web site I've been using every day with Nightly builds.

It would help the devs if you actually identified the web site (with URL).
(In reply to IU from comment #2)
> It would help the devs if you actually identified the web site (with URL).

Sorry, but it's the internal development version of my employer's product. If it was repeatable I would build a test case, but after two quick crashes this morning I haven't had a crash since. (And I've been doing the same thing over and over -- I have my own hard-to-reproduce bug to track down.)
It's currently #5 top crasher in 8.0b1.
Comments say it's related to Google or Gmail, maybe using quotes.
tracking-firefox8: --- → ?
Keywords: regression
Version: Trunk → 8 Branch
The regression range is:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=009c64b64cf3&tochange=c440edd84f84
The likely culprit is bug 668019.
Blocks: 668019
Summary: Firefox Crash @ nsDocShell::InternalLoad(nsIURI*, → Firefox Crash in nsDocShell::InternalLoad
(In reply to Scoobidiver from comment #5)
> The likely culprit is bug 668019.

Why likely? Did any comment mention that the user was copying the location bar value?
(In reply to Dão Gottwald [:dao] from comment #6)
> Why likely? Did any comment mention that the user was copying the location
> bar value?
Amongst the three bugs, this is the only one that calls the URI component.
Where did you get that range from?
The first entry in that table appears to be from September 7, which is outside of that range?
(In reply to Dão Gottwald [:dao] from comment #10)
> The first entry in that table appears to be from September 7, which is
> outside of that range?
It is the regression range for the spike (second?) because the crash daily rate is:
7.0.1: 0.13 crash/1M ADU
8.0a2 before 9/20: 60 crashes/1M ADU
8.0a2 after 9/21: 180 crashes/1M ADU
8.0b1: 380 crashes/1M ADU
I can't find regression ranges older than 30 days, so I don't know when the first spike appeared.
The first comment in the table says 'typed "furbush" in the google page', which as far as I can tell clearly rules out bug 668019. Spikes don't necessarily imply a regression -- this one could be due to a change on google's side.
No longer blocks: 668019
(In reply to Scoobidiver from comment #13)
> The regression range is:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=f414db34c70b&tochange=87e3ea12ed5d

Bug 646641? I think this would support the theory that there was a second spike due to a change at google.
Blocks: 646641
Bug 646641 is a reasonable guess for this crash, although it's hard to tell from these stacks what might be going wrong.

B.J., can you give us a little more information about the internal site you were using?  In particular, does it use history.push/replaceState or modify the URL's hash?
(In reply to Justin Lebar [:jlebar] from comment #15)
> B.J., can you give us a little more information about the internal site you
> were using?

Dojo, Struts, DWR. Lots of JavaScript to asynchronously load and save data.

> In particular, does it use history.push/replaceState or modify
> the URL's hash?

I don't think so. The application uses Dojo, and the Dojo code modifies location.hash in some cases. From a few searches I don't think our code uses dojo.hash, but Dojo has outsmarted me before.

I can't find any uses of history.push or history.replaceState.

I also have not seen this crash since the day I made my report in this discussion. I hope some code change is responsible for that. Is this crash signature still as common as it was?
(In reply to B.J. Herbison from comment #16)
> Is this crash signature still as common as it was?
Yes. It's #8 top crasher and #2 top crasher w/o hangs in 8.0b1.
>> In particular, does it use history.push/replaceState or modify
>> the URL's hash?
> I don't think so.

Thanks.

This makes it somewhat less likely that bug 646641 is at fault.  But if it's still our best guess, then it sounds like we should back it out and see if it fixes the crashes.

If I'm reading the crashstats page correctly, it looks like we'd need to back out from beta, since there are only three crashes that aren't on version 8.0.  bz, does backing out from beta seem reasonable to you?
For faster feedback, it can be backed out first in 10.0a1 where it's #12 top crasher.
(In reply to Scoobidiver from comment #19)
> For faster feedback, it can be backed out first in 10.0a1 where it's #12 top
> crasher.

Oh, I see.  Why don't those 10.0a1 crashes show up in the table in comment 0?
(In reply to Justin Lebar [:jlebar] from comment #20)
> Why don't those 10.0a1 crashes show up in the table in comment 0?
This query announces 1,793 crash reports but displays only 344 results in 8 pages!
Try run for 0112605a8509 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0112605a8509
Results (out of 171 total builds):
    exception: 1
    success: 141
    warnings: 17
    failure: 12
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-0112605a8509
I'm having difficulty bzexporting my backout patches at the moment, but smaug, can you sign off on backing out bug 646641 to see if it eliminates this crash?

        Attachment #566557 -
        Flags: review?(Olli.Pettay)

    
    

    
  


  

        Attachment #566559 -
        Flags: review?(Olli.Pettay)

        Attachment #566557 -
        Flags: review?(Olli.Pettay) → review+

    
    

    
  
Comment on attachment 566559 [details] [diff] [review]
Backout part 1, v1

rs for the backout in m-c

        Attachment #566559 -
        Flags: review?(Olli.Pettay) → review+

    
    

    
  
Backed out the backout because of mysterious moth permaorange which didn't appear on try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9670f22f912d

    
    

    
  
Try run for 194a7b7c1eff is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=194a7b7c1eff
Results (out of 24 total builds):
    success: 19
    warnings: 3
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-194a7b7c1eff

    
    

    
  
Maybe correlations by add-on can help to reproduce:
  nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, wchar_t const*, char const*, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**)|EXCEPTION_ACCESS_VIOLATION_READ (247 crashes)
     33% (81/247) vs.   0% (84/22826) azhang@cloudacl.com (adult content filtering)
     27% (67/247) vs.  11% (2457/22826) {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} (Adblock Plus, https://addons.mozilla.org/addon/1865)

    
    

    
  
Try run for ce38193e7b4b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ce38193e7b4b
Results (out of 24 total builds):
    success: 24
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-ce38193e7b4b

    
    

    
  
...and backed out the backout because it breaks Android.  I *will* get this right, eventually.

    
    

    
  
Try run for f297637346df is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f297637346df
Results (out of 15 total builds):
    success: 12
    warnings: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-f297637346df

    
    

    
  
Android looks ok.  This one's gonna work.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a34e6bed361
Whiteboard: [inbound]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8a34e6bed361

leaving open, as I understand this this is only backing out bug 646641, shouldn't that bug be reopened then?

    
    

    
  
Hm.  Well...now what?  I guess we start looking for places in InternalLoad which need null-checks?

    
    

    
  
Hm, maybe it's


+   // Link our new SHEntry to the old SHEntry's back/forward
+   // cache data, since the two SHEntries correspond to the
+   // same document.
+   mLSHE->AdoptBFCacheEntry(mOSHE);

This is the only place I see in [1] where we introduce a potential null-pointer exception.

Smaug, are you OK with me re-landing this patch with a null-check around this line?

[1] https://hg.mozilla.org/mozilla-central/diff/8a34e6bed361/docshell/base/nsDocShell.cpp

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/051b3d8ed545
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

    
  
Assignee: nobody → justin.lebar+bug

    
    

    
  
There are still no crashes from 10.0a1/20111023 when the new patch landed.

    
  

        Attachment #569090 -
        Flags: approval-mozilla-beta?

        Attachment #569090 -
        Flags: approval-mozilla-aurora?

    
    

    
  
---------------------------------[ Triage Comment ]---------------------------------

We are approving for both aurora9 and beta8 as the patch is said to be safe and fixes a top 20 crasher (release) and the #9 crash by volume in Firefox 8 beta 4.

Please land ASAP.

    
  

        Attachment #569090 -
        Flags: approval-mozilla-beta?

        Attachment #569090 -
        Flags: approval-mozilla-beta+

        Attachment #569090 -
        Flags: approval-mozilla-aurora?

        Attachment #569090 -
        Flags: approval-mozilla-aurora+

    
    

    
  
status-firefox9 should be fixed too, yes?

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #50)
> status-firefox9 should be fixed too, yes?

Yes, thanks.

          status-firefox9:
          affected → fixed

    
    

    
  
qa? as QA needs a testcase to verify this fix. Can a testcase be provided? Is merely checking crashstats sufficient to verify this fix?
Keywords: testcase-wanted
Whiteboard: [inbound] → [inbound][qa?]

    
    

    
  
We don't have a testcase.  I'd love it if you could create one, but I think it would be Hard.

    
    

    
  
Without a testcase we will need to rely on Socorro for assumed verification.
Whiteboard: [inbound][qa?] → [inbound][qa+]

    
    

    
  
http://bit.ly/u5HBSx

There still seem to be some crashes for Firefox 9 with the crash signature after the fix has landed. 

Is this expected in any way?

    
    

    
  
Most (all?) of the recent crashes seem to be segfaults with a non-null pointer, which would be a different issue from the one fixed here (and could point to general memory corruption, for all I know).  But maybe I'm reading crash-stats wrong.

    
    

    
  
(In reply to Justin Lebar [:jlebar] from comment #56)
> Most (all?) of the recent crashes seem to be segfaults with a non-null
> pointer, which would be a different issue from the one fixed here (and could
> point to general memory corruption, for all I know).  But maybe I'm reading
> crash-stats wrong.

(In reply to Justin Lebar [:jlebar] from comment #57)
> The crashers are also on different lines than the ones fixed here. 
> (InternalLoad is a big function.)

In that case, could you file new bugs for those that are reasonable enough volume (on trunk a couple occurrences are enough for that, probably)?

    
    

    
  
Someone who's better at navigating crash-stats should do this.

    
    

    
  
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #58)
> In that case, could you file new bugs for those that are reasonable enough
> volume (on trunk a couple occurrences are enough for that, probably)?
There are only crashes in 8.0* and 9.0 and, in both, it's beyond #300 crasher. I don't think they deserve a bug without a testcase.

    
    

    
  
(In reply to Scoobidiver from comment #60)
> (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #58)
> > In that case, could you file new bugs for those that are reasonable enough
> > volume (on trunk a couple occurrences are enough for that, probably)?
> There are only crashes in 8.0* and 9.0 and, in both, it's beyond #300
> crasher. I don't think they deserve a bug without a testcase.

I agree to that. If we have a testcase or steps to reproduce, that changes things, but other than that, we should better concentrate on getting the top 50 or so signatures fixed.

    
    

    
  
qa- due to lack of steps/testcase -- please reset to qa+ if one can be provided.
Whiteboard: [inbound][qa+] → [inbound][qa-]
Depends on: 823187

          You need to log in
          before you can comment on or make changes to this bug.