Closed
Bug 314549
Opened 19 years ago
Closed 19 years ago
Various bugs involving containers not actually fixed for subframes
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta2
People
(Reporter: bzbarsky, Assigned: bryner)
References
Details
(Keywords: fixed1.8.1)
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
mscott
:
approval1.8rc2-
benjamin
:
approval1.8.1-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
dbaron
:
approval1.8.1+
|
Details | Diff | Splinter Review |
It looks like we never unset document and prescontext containers, prescontext link handlers, etc for subframes when their parent goes into bfcache. This means that bug 292960 and possibly bug 292961 are not fixed for subframes.
While this is not quite as bad as the original bug 292960 and bug 292961, it still has the basic problem that code (extensions, content code, session history code, web pages, etc) doesn't expect that sort of thing to be happening, so allowing it to happen rather scares me security-wise. Once the page a frame is in has been unloaded, there should be no more traversals in that frame.
To fix this, I think we need to walk the docshell tree like we do the presshell tree when putting a page in bfache and unset the containers and link handler on everything in that subtree. Restoring needs to restore the pointers. It should be pretty safe to do this, I would think.
Reporter | ||
Updated•19 years ago
|
Blocks: blazinglyfastback
Flags: blocking1.8rc1?
Updated•19 years ago
|
Flags: blocking1.8rc1? → blocking1.8rc2?
Comment 1•19 years ago
|
||
This impacts bug 314288, which effectively renders FastFind useless- imho critical.
Thanks,
Andi
Comment 2•19 years ago
|
||
We're not going to be able to get to this for the 1.5 release. After discussing this with bryner, we'd like to pursue this for the next follow on release so we can address the remaining issues with subframes whose parent goes into bfcache.
Flags: blocking1.8rc2? → blocking1.8rc2-
Comment 3•19 years ago
|
||
If we don't fix this in 1.5 release, we have major bug(bug 314288).
That has low risk workaround patch. We should use it!!
Reporter | ||
Comment 4•19 years ago
|
||
For what it's worth, and this is the last time I'm going to worry about any bugs related to any of this, I think it would be worth considering preffing bfcache off by default and preffing it back on at the same time as we fix this bug, assuming we fix it in branch. I do think this is that serious.
Then again, given the amount of hoopla we've generated about bfcache in the last few months, I can see how it would be rather politically inexpedient to ship 1.5 without it. So I rather doubt the above course of action will be adopted.
Comment 5•19 years ago
|
||
There seems to be a disconnect as too how serious this issue is. Bryner and others don't think this is that bad and is best addressed in the follow up release. It would have to be a pretty serious show stopper for us to slip the release for this and I'm not seeing enough evidence that we should based on the feedback we've gotten.
Comment 6•19 years ago
|
||
Clearly I'm missing some context here, I assume stuff happened in private e-mail or something. Is this a security bug or not? bz's comments regarding the seriousness of this bug seem way out of alignment with the description.
What are the steps to reproduce?
Reporter | ||
Comment 7•19 years ago
|
||
Quote from a mail I sent earlier today (and really my last advocacy-like commment on this bug):
If anything demonstrates a problem with this code, it'll be a security exploit, not a site misbehaving.
And I can't even state categorically that there _is_ a way to exploit it, without doing a lot more legwork than I really have time or desire for. All I can say for sure is that we're now allowing certain actions on the part of script where we've never allowed them before. And that scares me. But maybe that's just me being paranoid.
Assignee | ||
Comment 8•19 years ago
|
||
This should take care of the problem, I think.
Attachment #201949 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 9•19 years ago
|
||
Comment on attachment 201949 [details] [diff] [review]
fix
>Index: layout/base/nsDocumentViewer.cpp
>@@ -1396,6 +1483,7 @@ DocumentViewerImpl::Destroy()
>+ nsISHEntry *shEntry = mSHEntry; // save a weak reference, see below
I don't really see what ensures that the shEntry stays alive. Please make that an nsCOMPtr, ok?
r=bzbarsky with that change. Thanks for fixing this, Brian!
Attachment #201949 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•19 years ago
|
||
checked in on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•19 years ago
|
||
Comment on attachment 201949 [details] [diff] [review]
fix
I think we can at least evaluate this for branch landing.
Attachment #201949 -
Flags: approval1.8rc2?
Comment 12•19 years ago
|
||
we've already got final bits in and are running our tests now. this has just landed on the trunk and not yet been verified there. can one of you tell us what the specific impact of this fix is and what kind of risk it poses?
Updated•19 years ago
|
Flags: blocking1.8rc2- → blocking1.8rc2?
Assignee | ||
Comment 13•19 years ago
|
||
I think Boris has already stated the impact/risk of this bug to the extent that we know.
My feeling is that exploiting this as a security bug would be difficult. As long as we leave bfcache disabled for subframe navigations, I can't see a way that page content could retain a reference to one of these subframes that's in bfcache but isn't completely unhooked. That leaves us with the possibility of built-in or privileged extension code getting confused. We don't touch content viewers in session history except to evict or restore them, so I'd consider that fairly unlikely as well... a malicious extension could do something here, but that's nothing new.
Unless anyone sees other avenues to explot this, I'd be inclined to let this bake on the trunk and pull it into the first 1.5.x bugfix release.
Reporter | ||
Comment 14•19 years ago
|
||
> As long as we leave bfcache disabled for subframe navigations
That doesn't affect this bug, since this bug pops up when we bfcache a toplevel page that has subframes.
Content could trivially retain a reference to one of these subframes -- just take a reference to a subframe and then load something in the toplevel page.
Reporter | ||
Comment 15•19 years ago
|
||
Impact of this fix:
When a page which has subframes goes into bfcache, in addition to setting pointers to the docshell to null on the toplevel page we also do so on all subframes. When we restore from bfcache, we go through and set all the pointers correctly again.
There is no impact on pages that have no subframes.
For pages with subframes the change should be very safe. It makes subframes of a page in bfcache pretty much identical in terms of behavior to the page itself; without this patch they behave a lot more like subframes of a page that is _not_ in bfcache. So the risk is minimal.
I agree with bryner that there is no obvious exploit (at least I haven't managed to come up with one yet, though there was the idea of one that I mentioned in some email recently), so if we're really far too late in the game, I suppose I'd be ok with shipping this in a security release (1.8.0.x, not just 1.8.x) down the road. This doesn't change any APIs, so that should be ok...
Assignee | ||
Comment 16•19 years ago
|
||
(In reply to comment #14)
> > As long as we leave bfcache disabled for subframe navigations
>
> That doesn't affect this bug, since this bug pops up when we bfcache a toplevel
> page that has subframes.
>
> Content could trivially retain a reference to one of these subframes -- just
> take a reference to a subframe and then load something in the toplevel page.
>
And then the script retaining the reference is either unloaded or put into bfcache, which in neither case would allow it to do anything with the reference.
Reporter | ||
Comment 17•19 years ago
|
||
> And then the script retaining the reference is either unloaded or put into
> bfcache
Not if it's in a different window altogether... As in, window.open a framed page, take a ref to a frame in that page, load something new in that window.
Comment 18•19 years ago
|
||
Comment on attachment 201949 [details] [diff] [review]
fix
unfortunately it's too late for the 1.5 train. Lets focus on getting this into the follow up release.
Attachment #201949 -
Flags: approval1.8rc2? → approval1.8rc2-
Updated•19 years ago
|
Flags: blocking1.8rc2? → blocking1.8.1?
Assignee | ||
Comment 19•19 years ago
|
||
Comment on attachment 201949 [details] [diff] [review]
fix
Would like to get this in for the 1.8 branch.
Attachment #201949 -
Flags: approval1.8.1?
Comment 20•19 years ago
|
||
Should this also go on the 1.8.0 branch?
Reporter | ||
Comment 21•19 years ago
|
||
This patch involves an API change, so for the 1.8.0 branch it'd need to be done differently, somehow. :(
Even for the 1.8.1 branch it might be worth looking for a way without the API change, though there it may be less of a problem.
But yes, in general I think this is worth fixing on the 1.8.0 branch.
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8.0.2?
Comment 22•19 years ago
|
||
Comment on attachment 201949 [details] [diff] [review]
fix
Can't take this as-is because of interface changes.
Attachment #201949 -
Flags: approval1.8.1? → approval1.8.1-
Comment 23•19 years ago
|
||
It's not looking like this would make 1.8.0.2, feel free to renominate if you can make the case and have a branch-safe patch.
Flags: blocking1.8.0.2? → blocking1.8.0.2-
Comment 24•19 years ago
|
||
We're interested in this for the 1.8 branch, but we'd need a version of the patch that does not break APIs.
Flags: blocking1.8.1? → blocking1.8.1+
Comment 25•19 years ago
|
||
Based on dveditz's willingness to take this as a security and stability update, it sounds like the sort of thing that we can take in beta2, but we'll need that API safe patch!
Target Milestone: --- → mozilla1.8.1beta2
Assignee | ||
Comment 26•19 years ago
|
||
We could do it this way... it's not really ideal since the old method stops working, but there's no way to implement it correctly without that SHEntry parameter
Attachment #228327 -
Flags: superreview?(darin)
Attachment #228327 -
Flags: review?(darin)
Attachment #228327 -
Flags: approval1.8.1?
Updated•19 years ago
|
Attachment #228327 -
Flags: superreview?(darin)
Attachment #228327 -
Flags: superreview+
Attachment #228327 -
Flags: review?(darin)
Attachment #228327 -
Flags: review+
Updated•19 years ago
|
Attachment #228327 -
Flags: approval1.8.1? → approval1.8.1+
Comment 28•19 years ago
|
||
The branch-"safe" fix caused SeaMonkey to go yellow on Linux and Windows:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.8-SeaMonkey
which I verified by backing out the patch locally on one build as starred.
Comment 29•19 years ago
|
||
Comment on attachment 228327 [details] [diff] [review]
branch-safe fix
>+ nsISHEntry *shEntry = mSHEntry; // save a weak reference, see below
>+ while (NS_SUCCEEDED(shEntry->ChildShellAt(itemIndex++,
>+ getter_AddRefs(item))) && item) {
My debug build dies here because shEntry is stale.
Reporter | ||
Comment 30•19 years ago
|
||
Hmm... Why _is_ that a weak ref? That seems wrong, since we immediately do a release on the shentry, so it could well go away. The "see below" is not helpful, since nowhere do we explain why that needs to be a weak ref.
Comment 31•19 years ago
|
||
*** Bug 345151 has been marked as a duplicate of this bug. ***
Comment 32•19 years ago
|
||
This crash blocks any of the automated testing of the js test library and related tests. Please fix as soon as possible or back it out. Thanks.
Comment 33•19 years ago
|
||
(In reply to comment #32)
> This crash blocks any of the automated testing of the js test library and
> related tests. Please fix as soon as possible or back it out. Thanks.
To clarify, the crash I am seeing when attempting to run tests is on the 1.8 branch due to the checkin in comment 27.
Assignee | ||
Comment 34•19 years ago
|
||
Hm, is the crash branch-only, or on trunk too? (the code is the same) And why doesn't it affect Firefox?
Comment 35•19 years ago
|
||
(In reply to comment #34)
> Hm, is the crash branch-only, or on trunk too? (the code is the same) And why
> doesn't it affect Firefox?
1.8 branch only, windows and linux but not Mac OS X ppc or intel. I don't know why it doesn't affect Firefox by itself. The steps to reproduce are in bug 345151 which I've cc'd you on.
Reporter | ||
Comment 36•19 years ago
|
||
Given the way we seem to crash, it could depend on GC timing amongst other things...
Assignee | ||
Comment 37•19 years ago
|
||
Well that's what I get for merging the patch I posted, rather than the one I checked in (which has this as a strong ref, based on Boris's review comment about this exact situation). Doh.
Attachment #229880 -
Flags: superreview?(bzbarsky)
Attachment #229880 -
Flags: review?(bzbarsky)
Attachment #229880 -
Flags: approval1.8.1?
Reporter | ||
Comment 38•19 years ago
|
||
Comment on attachment 229880 [details] [diff] [review]
crash fix
That should be an nsCOMPtr, right?
Assignee | ||
Comment 39•19 years ago
|
||
yes.
Attachment #229880 -
Attachment is obsolete: true
Attachment #229901 -
Flags: superreview?(bzbarsky)
Attachment #229901 -
Flags: review?(bzbarsky)
Attachment #229901 -
Flags: approval1.8.1?
Attachment #229880 -
Flags: superreview?(bzbarsky)
Attachment #229880 -
Flags: review?(bzbarsky)
Attachment #229880 -
Flags: approval1.8.1?
Comment 40•19 years ago
|
||
(In reply to comment #39)
appears to have fixed my crash on windows xp at least.
Reporter | ||
Comment 41•19 years ago
|
||
Comment on attachment 229901 [details] [diff] [review]
crash fix
r+sr=bzbarsky
Attachment #229901 -
Flags: superreview?(bzbarsky)
Attachment #229901 -
Flags: superreview+
Attachment #229901 -
Flags: review?(bzbarsky)
Attachment #229901 -
Flags: review+
Reporter | ||
Comment 42•19 years ago
|
||
*** Bug 345226 has been marked as a duplicate of this bug. ***
Comment 43•19 years ago
|
||
This allegedly caused bug 345083. Does the "crash fix" patch fix that too?
Comment 44•19 years ago
|
||
Comment on attachment 229901 [details] [diff] [review]
crash fix
a=dbaron on behalf of drivers. Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.
Attachment #229901 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 45•19 years ago
|
||
crash fix checked in on branch, was already fine on the trunk
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Comment 46•19 years ago
|
||
*** Bug 345306 has been marked as a duplicate of this bug. ***
Comment 47•19 years ago
|
||
(In reply to comment #43)
> This allegedly caused bug 345083. Does the "crash fix" patch fix that too?
>
The "crash fix" has indeed fixed bug 345083.
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in
before you can comment on or make changes to this bug.
Description
•