1251361 - "Assertion failure: cache->PreservingWrapper()" with <marquee>, navigation, adoptNode

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: cache->PreservingWrapper(), at js/xpconnect/wrappers/XrayWrapper.cpp:1191

(Initially security-sensitive because wrappers are security code.)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrew McCreight [:mccr8]]

It looks like Jon added this assertion.

Comment 3

[Jon Coppeard (:jonco) (PTO until Monday 29th)]

Unfortunately I'm not going to get a chance to take a proper look at this before I go away on PTO, so I'm going to punt on this.  Also I don't know really all that much about how this works.

I did reproduce it under rr and found that the wrapper was previously preserved, but was released here:

#0  0x00007f4257f667c2 in nsWrapperCache::UnsetWrapperFlags (this=0x7f4229bb6048, aFlagsToUnset=1)
    at /home/jon/clone/modules/dom/base/nsWrapperCache.h:313
#1  0x00007f4257f665a9 in nsWrapperCache::SetPreservingWrapper (this=0x7f4229bb6048, aPreserve=false)
    at /home/jon/clone/modules/dom/base/nsWrapperCache.h:184
#2  0x00007f4258bbec64 in nsWrapperCache::ReleaseWrapper (this=0x7f4229bb6048, aScriptObjectHolder=0x7f4229bb6040)
    at /home/jon/clone/modules/dom/base/nsWrapperCache.cpp:60
#3  0x00007f4258984643 in mozilla::dom::FragmentOrElement::DestroyContent (this=0x7f4229bb6040)
    at /home/jon/clone/modules/dom/base/FragmentOrElement.cpp:1191
#4  0x00007f425898468c in mozilla::dom::FragmentOrElement::DestroyContent (this=0x7f42297369c0)
    at /home/jon/clone/modules/dom/base/FragmentOrElement.cpp:1196
#5  0x00007f425898468c in mozilla::dom::FragmentOrElement::DestroyContent (this=0x7f422edb9540)
    at /home/jon/clone/modules/dom/base/FragmentOrElement.cpp:1196
#6  0x00007f4258a95205 in nsDocument::Destroy (this=0x7f4240612000)
    at /home/jon/clone/modules/dom/base/nsDocument.cpp:8891
#7  0x00007f425ada390e in nsDocumentViewer::Destroy (this=0x7f4229b08080)
    at /home/jon/clone/modules/layout/base/nsDocumentViewer.cpp:1644
#8  0x00007f425ada4af0 in nsDocumentViewer::Show (this=0x7f4229b087a0)
    at /home/jon/clone/modules/layout/base/nsDocumentViewer.cpp:1959
#9  0x00007f425add9358 in nsPresContext::EnsureVisible (this=0x7f422eeed000)
    at /home/jon/clone/modules/layout/base/nsPresContext.cpp:2245
#10 0x00007f425ade789b in PresShell::UnsuppressAndInvalidate (this=0x7f422985ec00)
    at /home/jon/clone/modules/layout/base/nsPresShell.cpp:3744
#11 0x00007f425ade7a62 in PresShell::UnsuppressPainting (this=0x7f422985ec00)
    at /home/jon/clone/modules/layout/base/nsPresShell.cpp:3787
#12 0x00007f425ada0fd9 in nsDocumentViewer::LoadComplete (this=0x7f4229b087a0, aStatus=nsresult::NS_OK)
    at /home/jon/clone/modules/layout/base/nsDocumentViewer.cpp:1031
#13 0x00007f425b47fa9f in nsDocShell::EndPageLoad (this=0x7f422eedb800, aProgress=0x7f422eedb828, aChannel=
    0x7f422974b820, aStatus=nsresult::NS_OK) at /home/jon/clone/modules/docshell/base/nsDocShell.cpp:7506

nsWrapperCache::ReleaseWrapper clears the expando object if the wrapper is a proxy, which this one isn't.

If the above is expected I guess the assertion is wrong.

Comment 4

[Andrew Overholt [:overholt]]

Peter or Bobby, can check this out before Jon gets back from PTO?

Comment 5

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

oh, fun, I think that ReleaseWrapper(this); call used to be a possible security bug before we started to automatically drop all the C++->JS edges when DropJSObjectsImpl is called
(nsWrapperCache::ReleaseWrapper calls that, and we end up to call tracer->Trace(aHolder, ClearJSHolder(), nullptr)). Luckily that got fixed long ago.

From memory usage point of view that ReleaseWrapper call is kind of nice. Kill certain edges explicitly rather early, no need to use CC/GC for that. But of course it isn't quite right from correctness point of view. The ReleaseWrapper call was added there before we even had proper
CC handling for the whole DOM tree (strong parentNode etc. Bug 335998), so it might be safe to remove it these days. hueyfix (Bug 695480) could have also helped with bug 406684.

Comment 6

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

I don't know whether removing that ReleaseWrapper call fixes this, but
at least it is good to know whether we still leak without it
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17e0274bb0b8

Comment 7

[Bobby Holley (:bholley)]

I don't have the cycles to get involved right now.

Comment 8

[Peter Van der Beken [:peterv]]

I'll take a look.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> I'll take a look.

Thanks peter :-)

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] (vacation Feb 29 - Mar 4) from comment #6)
> I don't know whether removing that ReleaseWrapper call fixes this, but
> at least it is good to know whether we still leak without it
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=17e0274bb0b8

It looks like we still leak. That would be interesting to investigate at some point.

I'll mark this sec-moderate, as Olli said we neuter the JS pointers out of this object anyways, but maybe somebody could do something weird with it. If this is wrong, feel free to adjust or clear the rating.

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> I'll mark this sec-moderate, as Olli said we neuter the JS pointers out of
> this object anyways, but maybe somebody could do something weird with it. If
> this is wrong, feel free to adjust or clear the rating.

I don't think the fact that we're not preserving the reflector is a security issue. We'll just not keep the reflector alive, so it could get collected and we'll collect the expando object (losing any expando properties). The expando objects are stored in a weak map (mapping reflector -> expando).
I'm tracking down the leaks, I'll file separate bugs for those.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 13

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

Comment on attachment 8737504 [details] [diff] [review]
v1

>+++ b/widget/tests/test_bug673301.xul
>@@ -28,11 +28,13 @@ var transferable = Components.classes['@
>                              .createInstance(Components.interfaces.nsITransferable);
> transferable.init(getLoadContext());
> 
> transferable.addDataFlavor("text/unicode");
> transferable.setTransferData("text/unicode", document, 4);
> 
> clipboard.setData(transferable, null, Components.interfaces.nsIClipboard.kGlobalClipboard);
> 
>+transferable.setTransferData("text/unicode", null, 0);
ok, this test is totally artificial case. So I'm not too worried about this change.

But should we still consider making nsTransferable CCable?
Perhaps in a followup bug and then backout the change to this test?

Because this is a scary change (might reveal some leaks), might be good to land now, but backout asap from the next
Aurora if we see new leaks.

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to Olli Pettay [:smaug] from comment #13)
> But should we still consider making nsTransferable CCable?
> Perhaps in a followup bug and then backout the change to this test?

I had looked into it. Transferable might be doable, but the annoying part will be everything that holds on to it and further. The clipboard stuff for example has implementation per platform. I'll file a followup.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54ee322b50cf

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b776d4ae2e72 for apparently causing some leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=25097305&repo=mozilla-inbound

Comment 17

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8c582fe335d5ed99ae5b79593f287b3c62edae
Bug 1251361 - "Assertion failure: cache->PreservingWrapper()" with <marquee>, navigation, adoptNode. r=smaug.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b8c582fe335

Comment 20

[Peter Van der Beken [:peterv]]

Leak was fixed in bug 1262110.