1247872 - crash in nsILoadContext::UsePrivateBrowsing

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[[:philipp]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Null-check the result of GetLoadContext() when doing frame targeting, even though it really should never be null there... because apparently sometimes it is

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated per IRC discussion

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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;
  }

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, in fairness aAccessingItem might be an nsWebBrowser.  But aTargetItem certainly won't be, I would think.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

oh, right. then add null check.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: 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...

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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() ||

Comment 10

[Pulsebot]

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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2cacb0f32a4c

Comment 12

[[:philipp]]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think that would probably be ok, yes.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.