1473507 - Crash in nsILoadInfo::GetOriginAttributes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-c265d263-1cbf-4055-9bcb-1b23d0180704.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsILoadInfo::GetOriginAttributes dist/include/nsILoadInfo.h:441
1 xul.dll mozilla::net::HttpBaseChannel::SetReferrerWithPolicy netwerk/protocol/http/HttpBaseChannel.cpp:1679
2 xul.dll nsWebBrowserPersist::SaveURIInternal dom/webbrowserpersist/nsWebBrowserPersist.cpp:1401
3 xul.dll nsWebBrowserPersist::SavePrivacyAwareURI dom/webbrowserpersist/nsWebBrowserPersist.cpp:445
4 xul.dll XPTC__InvokebyIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
5  @0xfff9ffffffffffff 
6 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1186
7 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:893
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:531
9 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3195

=============================================================

There are 45 crashes (from 23 installations) in nightly 63 starting with buildid 20180704100142. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1469916.

[1] https://hg.mozilla.org/mozilla-central/rev?node=9a2b02fe351b

Comment 1

[Ekanan Ketunuti]

STR:

1. load https://upload.wikimedia.org/wikipedia/commons/3/3d/LARGE_elevation.jpg
2. click at 

Comment 2

[Ekanan Ketunuti]

STR:

1. load https://upload.wikimedia.org/wikipedia/commons/3/3d/LARGE_elevation.jpg
2. click at info icon (i in circle) on the left of urlbar
3. select >
4. select more infomation
5. select media
6. save https://upload.wikimedia.org/wikipedia/commons/3/3d/LARGE_elevation.jpg

Booooooooom!!!! bp-017cf625-9fc6-4bf9-bfc8-c99ab0180705

Comment 3

[:Gijs (he/him)]

How is it possible for a channel to have a null loadinfo (or for that to have null origin attributes)?

Also not sure how my patch triggers this...

Comment 4

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: regression, #3 top crash on the July 4 Windows Nightly.

Comment 5

[:Gijs (he/him)]

(In reply to Ekanan Ketunuti from comment #2)
> STR:
> 
> 1. load
> https://upload.wikimedia.org/wikipedia/commons/3/3d/LARGE_elevation.jpg
> 2. click at info icon (i in circle) on the left of urlbar
> 3. select >
> 4. select more infomation
> 5. select media
> 6. save
> https://upload.wikimedia.org/wikipedia/commons/3/3d/LARGE_elevation.jpg
> 
> Booooooooom!!!! bp-017cf625-9fc6-4bf9-bfc8-c99ab0180705

Thanks, this helps. I'll just sellotape over this...

You know, you'd think that if something were called `internalSave`, people wouldn't call it willy-nilly from various end-consumers all over the codebase. But you'd be wrong...

Comment 6

[:Gijs (he/him)]

(also, you'd think there were automated test, and you'd be wrong about that too...)

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1473507 - fix crash in nsILoadInfo::GetOriginAttributes when passing no principal to SavePrivacyAwareURI,

Review commit: https://reviewboard.mozilla.org/r/255148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255148/

Comment 8

[:Gijs (he/him)]

I went over https://dxr.mozilla.org/mozilla-central/search?q=saveURL+-path%3Askia+-path%3Atest&redirect=false and https://dxr.mozilla.org/mozilla-central/search?q=internalSave(+-path%3Askia+-path%3Atest&redirect=false another time, and I don't *think* there's any other consumers that don't pass a principal or document anymore, but at least with this patch we should stop crashing in this particular spot...

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8990149 [details]
Bug 1473507 - fix crash in nsILoadInfo::GetOriginAttributes when passing no principal to SavePrivacyAwareURI,

https://reviewboard.mozilla.org/r/255148/#review262046

This needs more of an explanation in the commit message. I guess the C++ change is a null check on an argument, but what is the JS change doing?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8990149 [details]
Bug 1473507 - fix crash in nsILoadInfo::GetOriginAttributes when passing no principal to SavePrivacyAwareURI,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255148/diff/1-2/

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8990149 [details]
Bug 1473507 - fix crash in nsILoadInfo::GetOriginAttributes when passing no principal to SavePrivacyAwareURI,

https://reviewboard.mozilla.org/r/255148/#review262050

Thanks.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddb29b481834
fix crash in nsILoadInfo::GetOriginAttributes when passing no principal to SavePrivacyAwareURI, r=mccr8

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ddb29b481834