crash in nsILoadContext::UsePrivateBrowsing

RESOLVED FIXED in Firefox 47

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: philipp, Assigned: bzbarsky)

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

3 years 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
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-
Posted patch Updated per IRC discussion (obsolete) — Splinter Review
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.
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 11

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

Comment 12

3 years 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.