Closed Bug 1251361 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: XPConnect, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jruderman, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: btpp-active)

Attachments

(3 files)

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

(Initially security-sensitive because wrappers are security code.)
Attached file stack
It looks like Jon added this assertion.
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(jcoppeard)
Peter or Bobby, can check this out before Jon gets back from PTO?
Flags: needinfo?(peterv)
Flags: needinfo?(bobbyholley)
Whiteboard: btpp-followup-2016-03-04
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.
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
I don't have the cycles to get involved right now.
Flags: needinfo?(bobbyholley)
I'll take a look.
Assignee: nobody → peterv
Flags: needinfo?(peterv)
(In reply to Peter Van der Beken [:peterv] from comment #8)
> I'll take a look.

Thanks peter :-)
(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.
Keywords: sec-moderate
(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.
Depends on: 1253641
Group: dom-core-security
Keywords: sec-moderate
Whiteboard: btpp-followup-2016-03-04 → btpp-active
Depends on: 1253734
Depends on: 1259044
Attached patch v1Splinter Review
Attachment #8737504 - Flags: review?(bugs)
Blocks: 1259044
No longer depends on: 1259044
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.
Attachment #8737504 - Flags: review?(bugs) → review+
(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.
Depends on: 1262110
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8c582fe335d5ed99ae5b79593f287b3c62edae
Bug 1251361 - "Assertion failure: cache->PreservingWrapper()" with <marquee>, navigation, adoptNode. r=smaug.
https://hg.mozilla.org/mozilla-central/rev/9b8c582fe335
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 1259044
Flags: needinfo?(peterv)

Leak was fixed in bug 1262110.

You need to log in before you can comment on or make changes to this bug.