Closed
Bug 468211
Opened 16 years ago
Closed 16 years ago
Crash [@ nsCSSFrameConstructor::AdjustParentFrame] with DOMAttrModified, observes, binding and focusing
Categories
(Core :: XUL, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: smaug)
References
Details
(5 keywords, Whiteboard: [sg:critical])
Crash Data
Attachments
(7 files)
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
text/plain; charset=UTF-8
|
Details | |
(deleted),
text/plain; charset=UTF-8
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
See testcase, which crashes current trunk build within 100ms.
This regressed between 2008-09-11 and 2008-09-12:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-11+10%3A00%3A00&enddate=2008-09-12+03%3A00%3A00
I think a regression from bug 454361 somehow.
http://crash-stats.mozilla.com/report/index/6e989257-4557-4111-aa89-7695d2081205?p=1
0 xul.dll nsCSSFrameConstructor::AdjustParentFrame layout/base/nsCSSFrameConstructor.cpp:3407
1 xul.dll nsCSSFrameConstructor::ConstructFrameInternal layout/base/nsCSSFrameConstructor.cpp:7494
2 xul.dll nsCSSFrameConstructor::ConstructFrame layout/base/nsCSSFrameConstructor.cpp:7406
3 xul.dll nsCSSFrameConstructor::ContentInserted layout/base/nsCSSFrameConstructor.cpp:8997
4 xul.dll nsCSSFrameConstructor::RecreateFramesForContent layout/base/nsCSSFrameConstructor.cpp:11143
5 xul.dll nsCSSFrameConstructor::ProcessRestyledFrames layout/base/nsCSSFrameConstructor.cpp:9868
6 xul.dll nsCSSFrameConstructor::RestyleElement layout/base/nsCSSFrameConstructor.cpp:9942
7 xul.dll nsCSSFrameConstructor::ProcessOneRestyle layout/base/nsCSSFrameConstructor.cpp:13280
8 xul.dll nsCSSFrameConstructor::ProcessPendingRestyles layout/base/nsCSSFrameConstructor.cpp:13378
9 xul.dll PresShell::DoFlushPendingNotifications layout/base/nsPresShell.cpp:4541
10 xul.dll PresShell::FlushPendingNotifications layout/base/nsPresShell.cpp:4505
11 xul.dll nsCSSFrameConstructor::RestyleEvent::Run layout/base/nsCSSFrameConstructor.cpp:13451
12 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:510
13 xul.dll nsBaseAppShell::Run widget/src/xpwidgets/nsBaseAppShell.cpp:170
14 nspr4.dll PR_GetEnv
15 firefox.exe wmain toolkit/xre/nsWindowsWMain.cpp:87
16 firefox.exe firefox.exe@0x2197
17 kernel32.dll BaseProcessStart
Reporter | ||
Comment 1•16 years ago
|
||
Because of the load delay by loading it over the network, there is a chance it doesn't crash. In that case, load the testcase in offline mode or test the testcase locally.
Flags: blocking1.9.1?
Reporter | ||
Updated•16 years ago
|
Severity: normal → critical
Comment 2•16 years ago
|
||
I don't see a crash on Linux, either from the network or from a local file, although I do see some assertions and warnings.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment 3•16 years ago
|
||
I crash on Windows from a local file, though.
Comment 4•16 years ago
|
||
The variable parentFrame in ContentInserted is full of 0xdddddddd, and then we crash when we start accessing it.
Comment 5•16 years ago
|
||
And the thing we're calling RecreateFramesForContent on is the tooltip element.
Reporter | ||
Comment 6•16 years ago
|
||
This is probably the same issue.
Reporter | ||
Updated•16 years ago
|
Summary: Crash [@ nsCSSFrameConstructor::AdjustParentFrame] with DOMAttrModified, binding and focusing → Crash [@ nsCSSFrameConstructor::AdjustParentFrame] with DOMAttrModified, observes, binding and focusing
Reporter | ||
Comment 7•16 years ago
|
||
Comment 8•16 years ago
|
||
testcase2 crashes reliably on Linux.
Comment 9•16 years ago
|
||
This is from debugging testcase2.
So I think the problem is related to this stack: we're removing the node here, and then on unwind from here, we're then trying to do frame construction on a node that's no longer in the document, which crashes.
Before this point, we get:
###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file ../../../dist/include/content/nsContentUtils.h, line 1528
called from what's frame #13 in this stack (but line 4336), but then we just go on and run the mutation events anyway, at this stack. I'd have though that the assertion would imply that we wouldn't run the mutation events given that some of the script blockers were not removable.
Comment 10•16 years ago
|
||
... in particular, we unwind to frame #24, and then get a pile of assertions when it calls ConstructFrameByDisplayType ... ConstructBlock ... ProcessChildren
But we actually don't crash until reflow, where we try to reflow some deleted frames.
Comment 11•16 years ago
|
||
Assignee: nobody → dbaron
Comment 12•16 years ago
|
||
But the crash is a result of having frames for an element with a null document pointer, because of the warning from RecreateFramesForContent:
WARNING: NS_ENSURE_TRUE(aContent->GetDocument()) failed: file /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10984
since this is in the middle of a style context rebuild, we end up crashing as a result of skipping recreation of frames, since we depended on the frame reconstruction to delete the style contexts with dangling pointers.
There are a few things that are fragile here that we could perhaps fix:
1. rule tree reconstruction assuming all pointers into the old rule tree will be destroyed
2. style reresolution relying on frame reconstruction to destroy all the frames with old style context pointers (and the resulting skipping of the style context pointer fixup)
but it does look like the underlying problem comes down to constructing frames for a node that's not in the document.
Comment 13•16 years ago
|
||
I don't like this; I'm going to rewrite it. But I want to save it here because it does have potentially useful child enumeration code (which I'm going to get rid of for the next version).
Comment 14•16 years ago
|
||
(In reply to comment #12)
> There are a few things that are fragile here that we could perhaps fix:
3. Make RecreateFramesForContent destroy the frames but not rebuild them in the case we're hitting.
The problem is somewhere in frame 15-18. We really shouldn't be calling SetAttr with aNotify=1 when it's unsafe to run script. One of the functioncalls in those frames need to be put in a scriptrunner. I'm guessing either the call to SynchronizeBroadcastListener or the calls to SetAttr.
There really is no other way to fully fix the crash here since the script in the mutation observer can change the whole world under the frame-ctor.
Neil, do you have an opinion on what to put off for when it's safe to run scripts? See above comment 15.
Since this can lead to exploitable crashes I'm marking this security sensitive.
Group: core-security
Whiteboard: [sg:critical]
Comment 18•16 years ago
|
||
I moved the business about asserting on EndReconstruct into bug 473871.
Comment 19•16 years ago
|
||
And I've decided I am going to fix the rule tree thing in bug 475128.
However, the script could have done much worse things here that layout can't protect against; this is really a content bug. (Sorry, should have moved this last week.)
Assignee: dbaron → nobody
Component: Layout → DOM
QA Contact: layout → general
Assignee | ||
Comment 20•16 years ago
|
||
I need to test if Bug 471166 fixes this one.
Assignee | ||
Comment 21•16 years ago
|
||
This doesn't crash anymore, at least not in trunk.
I get a "killing mutation events" assertion.
Assignee | ||
Comment 22•16 years ago
|
||
Ah, sorry, the testcase 3 does still crash :( And Bug 471166 doesn't help here.
Assignee | ||
Comment 23•16 years ago
|
||
Fixes also Bug 472668.
I need to still run all the tests.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Attachment #358815 -
Flags: superreview?(jonas)
Attachment #358815 -
Flags: review?(jonas)
Assignee | ||
Updated•16 years ago
|
Component: DOM → XUL
QA Contact: general → xptoolkit.widgets
Whiteboard: [sg:critical] → [sg:critical][needs review]
Comment 24•16 years ago
|
||
FWIW, the patch in bug 475128 didn't fix this. I get a crash in nsCSSFrameConstructor::ConstructFrameInternal when I load the testcase.
Attachment #358815 -
Flags: superreview?(jonas)
Attachment #358815 -
Flags: superreview+
Attachment #358815 -
Flags: review?(jonas)
Attachment #358815 -
Flags: review+
Assignee | ||
Comment 25•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.1
Assignee | ||
Updated•16 years ago
|
Whiteboard: [sg:critical][needs review] → [sg:critical]
Reporter | ||
Comment 26•16 years ago
|
||
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre (.NET CLR 3.5.30729)
Status: RESOLVED → VERIFIED
Comment 27•16 years ago
|
||
Shouldn't we take this one on 1.9.0.7 with the others (bug 466057 et al.) you've fixed like this?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.7?
Comment 28•16 years ago
|
||
Updated•16 years ago
|
Flags: blocking1.9.0.7? → blocking1.9.0.7+
Updated•16 years ago
|
Attachment #358815 -
Flags: approval1.9.0.7+
Comment 29•16 years ago
|
||
Comment on attachment 358815 [details] [diff] [review]
patch, requires bug 471166
Approved for 1.9.0.7, a=dveditz for release-drivers.
Assignee | ||
Comment 30•16 years ago
|
||
The patch depends on 1.9.1/trunk broadcasting changes.
Assignee | ||
Updated•16 years ago
|
Attachment #358815 -
Attachment description: patch, requires bug 471166 → patch, requires bug 471166
Attachment #358815 -
Flags: approval1.9.0.7+
Assignee | ||
Comment 31•16 years ago
|
||
Changing blocking1.9.0.7+ to blocking1.9.0.7?
Flags: blocking1.9.0.7+ → blocking1.9.0.7?
Updated•16 years ago
|
Flags: blocking1.9.0.7? → blocking1.9.0.8?
Comment 32•16 years ago
|
||
Can you stop this crash working around the missing trunk changes? Would it be hard to back-port the trunk broadcasting changes?
Comment 33•16 years ago
|
||
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5
Keywords: fixed1.9.1 → verified1.9.1
Comment 34•16 years ago
|
||
These self-referencing testcases no longer work from bugzilla. Is there something we could change on the server end, maybe let the documents be cached so the self-references don't trigger a re-load and therefore a re-redirect?
Comment 35•16 years ago
|
||
testcase3 (at least) is crashing in the same place in 1.9.0.8pre as reported here for trunk. We ought to get this fix into 1.9.0.8
Comment 36•16 years ago
|
||
(In reply to comment #34)
> These self-referencing testcases no longer work from bugzilla. Is there
> something we could change on the server end, maybe let the documents be cached
> so the self-references don't trigger a re-load and therefore a re-redirect?
I don't think so... the tokens used for security bugs are one-time-use-only.
Updated•16 years ago
|
Flags: blocking1.9.0.8? → blocking1.9.0.8+
Comment 37•16 years ago
|
||
Still need an answer to comment 32 from Olli, but clearly this isn't going to make tomorrow's code freeze regardless.
Flags: blocking1.9.0.8+ → blocking1.9.0.9+
Assignee | ||
Comment 38•16 years ago
|
||
(In reply to comment #37)
> but clearly this isn't going to
> make tomorrow's code freeze regardless.
Sorry about that.
I think all the broadcaster changes should be ported to branch. There are quite
a few... :/
Comment 39•16 years ago
|
||
Now that 3.5b4 is essentially done you've got time to roll-up all the broadcaster changes you're talking about? Not going to make 1.9.0.10 because that code-freezes next week, but please start working on it now so it doesn't miss another release.
Updated•16 years ago
|
Flags: blocking1.9.0.10+ → blocking1.9.0.11+
Assignee | ||
Comment 40•15 years ago
|
||
Keywords: fixed1.9.0.12
Assignee | ||
Updated•15 years ago
|
Keywords: fixed1.9.0.12
Assignee | ||
Comment 41•15 years ago
|
||
The assertions are fixed now in 1.9.0.12. I can't reliably reproduce the
crash on 1.9.0.x.
Martijn, could you help verifying this and bug 472668.
Keywords: fixed1.9.0.12
Comment 42•15 years ago
|
||
Testcase 3 crashes reliably for me on Windows XP with 3.0.11. With 3.0.12pre (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre), it does not crash. Marking verified1.9.0.12.
Other cases do not reproduce a crash.
Keywords: fixed1.9.0.12 → verified1.9.0.12
Updated•15 years ago
|
Group: core-security
Comment 43•15 years ago
|
||
Added crashtests, but had to disable one temporarily due to bug 509269.
http://hg.mozilla.org/mozilla-central/rev/b3f6bc8b17e8
Flags: in-testsuite+
Updated•13 years ago
|
Crash Signature: [@ nsCSSFrameConstructor::AdjustParentFrame]
You need to log in
before you can comment on or make changes to this bug.
Description
•