crash in nsILoadContext::UsePrivateBrowsing

RESOLVED FIXED in Firefox 47

Status

()

Core
Document Navigation
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: philipp, Assigned: bz)

Tracking

({crash, regression})

43 Branch
mozilla47
x86
Windows NT
crash, regression
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 affected, firefox46+ wontfix, firefox47 fixed)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-855b608e-5a34-4db6-8860-9a9bd2160126.
=============================================================
0 	xul.dll 	nsILoadContext::UsePrivateBrowsing() 	obj-firefox/dist/include/nsILoadContext.h
1 	xul.dll 	nsDocShell::CanAccessItem(nsIDocShellTreeItem*, nsIDocShellTreeItem*, bool) 	docshell/base/nsDocShell.cpp
2 	xul.dll 	nsDocShell::CanAccessItem(nsIDocShellTreeItem*, nsIDocShellTreeItem*, bool) 	docshell/base/nsDocShell.cpp
3 	xul.dll 	nsDocShell::DoFindItemWithName(wchar_t const*, nsISupports*, nsIDocShellTreeItem*, nsIDocShellTreeItem**) 	docshell/base/nsDocShell.cpp
4 	xul.dll 	nsDocShell::FindItemWithName(wchar_t const*, nsISupports*, nsIDocShellTreeItem*, nsIDocShellTreeItem**) 	docshell/base/nsDocShell.cpp

this signature seems to be a regression since 43 builds.
just guessing on timing, it may be related to the landing of bug 1100154 perhaps?
Looks like it.  For a document in a docshell the loadinfo should really never be null (since it's a pointer to the docshell itself), but apparently it sometimes is.... <sigh>.
Assignee: nobody → bzbarsky
Component: Private Browsing → Document Navigation
Product: Firefox → Core
Created attachment 8718845 [details] [diff] [review]
Null-check the result of GetLoadContext() when doing frame targeting, even though it really should never be null there... because apparently sometimes it is
Attachment #8718845 - Flags: review?(bugs)
Comment on attachment 8718845 [details] [diff] [review]
Null-check the result of GetLoadContext() when doing frame targeting, even though it really should never be null there... because apparently sometimes it is

So I think we shouldn't use GetDocument at all, but just 
QI aAccessingItem and aAccessingItem to loadcontext.
Or static cast targetDS and accessingDS to nsDocShell and use UsePrivateBrowsing() on them.
Attachment #8718845 - Flags: review?(bugs) → review-
Created attachment 8718931 [details] [diff] [review]
Updated per IRC discussion
Attachment #8718931 - Flags: review?(bugs)
Attachment #8718845 - Attachment is obsolete: true
Comment on attachment 8718931 [details] [diff] [review]
Updated per IRC discussion

And as far as I see targetDS and accessingDS are never null here, even though there is a null check in the 'if' above.

There is
  if (!gValidateOrigin || !aAccessingItem) {
    // Good to go
    return true;
  }
and then 
  if (!!targetDS != !!accessingDS) {
    // We must be able to convert both or neither to nsIDocShell.
    return false;
  }
Attachment #8718931 - Flags: review?(bugs) → review+
Well, in fairness aAccessingItem might be an nsWebBrowser.  But aTargetItem certainly won't be, I would think.
oh, right. then add null check.
Created attachment 8718978 [details] [diff] [review]
Just get our private browsing state directly off the docshells we already have instead of trying to indirect through their documents

Sorry, one more time just to make sure we're on the same page here...
Attachment #8718978 - Flags: review?(bugs)
Attachment #8718931 - Attachment is obsolete: true
Comment on attachment 8718978 [details] [diff] [review]
Just get our private browsing state directly off the docshells we already have instead of trying to indirect through their documents

In theory someone could pass webbrowser as a param to FindChildWithName, but
I think that shouldn't really return anything.
WebBrowser's relevant method returns always null.

align           accessingDS->GetIsInBrowserElement() ||
Attachment #8718978 - Flags: review?(bugs) → review+

Comment 10

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cacb0f32a4c

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2cacb0f32a4c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Reporter)

Comment 12

a year ago
in 46.0b8 this crash is currently on #23 making up 0.56% of all crashes. do you think that the patch would be uplift-able to beta in terms of scope and risk?
tracking-firefox46: --- → ?
Flags: needinfo?(bzbarsky)
I think that would probably be ok, yes.
Flags: needinfo?(bzbarsky)
I missed this last week and now we're heading into 46 beta 11. 
I think it may be best to let this ride with 47.
status-firefox46: affected → wontfix
tracking-firefox46: ? → +
You need to log in before you can comment on or make changes to this bug.