Summary: | Using jQuery to show/hide IMG elements crashes WebKit | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||||||||||||||||||||||||||||||||||
Component: | Images | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | andrey.petrov, ap, aroben, commit-queue, darin, dbates, dglazkov, enrica, jberlin, joone, mitz, rniwa, simon.fraser, webkit.review.bot, yael | ||||||||||||||||||||||||||||||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Mihnea Ovidenie
2009-11-20 06:10:11 PST
Created attachment 43575 [details]
Crash log on Mac
Created attachment 43576 [details]
Dr watson log on WinXP
Created attachment 43577 [details]
User dmp on WinXP
Created attachment 43844 [details]
Simple HTML test file, no jquery involved
I made a simplified version of the crash and tested with latest nighlty 51303 on Windows. This time, no jquery involved. Basically, the code that crashes WebKit is as follows:
<script>
function openPreferences() {
setTimeout(function() {
document.getElementById('DIV').style.display = 'none';
}, 100);
}
</script>
<body onmousedown="openPreferences()">
<div>
<div ID="DIV">
<div> <img id="IMG" src="5day.png"/> </div>
</div>
</div>
</body>
If hiding (or removing) the image from DOM is done on a timer, then the right click menu can be displayed and in addition to that, when the CopyImage action is chosen, Webkit crashes since the renderer for the node is 0.
Regards,
Mihnea
I've tried this on windows and linux/gtk and it crashes all the same. The reason is indeed renderer set to NULL as the detachment already happened. I tried to reproduce this issue (copying image of a node into clipboard) with some other trigger (.removeChild on button handler) and naturally it crashes the same way. I think the right way to fix this issue would be to remove superfluous ASSERTs and if renderer absents just exit gracefully. I grepped code all around and in many places existence of a renderer for a given node is not assumed and a fallback is implemented. Moreover Android version already checks if render is 0 so it would appear to be the right way. I tried to reproduce a case where the NODE actually could be deleted by the time action is activated. I did not succeed no matter how I tried to kill the node with javascript or even with meta refresh. So I think it is safe to leave assertion for node. Created attachment 93951 [details]
proposed patch
proposed trivial patch for gtk, chromium, qt, win and WinCE
Comment on attachment 93951 [details] proposed patch Attachment 93951 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8705774 Created attachment 93960 [details]
trival patch
trivial patch, fixed a build error;)
Created attachment 93997 [details]
proposed patch, take 3
added Mac
Comment on attachment 93997 [details] proposed patch, take 3 Attachment 93997 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8710644 Created attachment 93998 [details]
trivial patch
again broken Qt and fixed it again
Comment on attachment 93998 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=93998&action=review This bug still affects Mac Safari as of version 5.0.5 (6533.21.1, r86784). But I don't see any changes in this patch for the Mac port. > Source/WebCore/ChangeLog:8 > + No new tests. Can you elaborate on why we can't test this? It seems like we could at least have a manual test case. > Source/WebCore/ChangeLog:10 > + For platform code, do not assert nodes always have renderer. It would be beneficial if you could elaborate a bit on this. (In reply to comment #13) thanks for looking over this. > Can you elaborate on why we can't test this? It seems like we could at least > have a manual test case. Yeah this is pretty easily testable. The test case is actually available in this bug. I thought only automated test cases can be mentioned. Will fix it. > > > Source/WebCore/ChangeLog:10 > > + For platform code, do not assert nodes always have renderer. > > It would be beneficial if you could elaborate a bit on this. please look at comment #6. If the reasoning sounds good I can of course paste it into the changelog. (In reply to comment #13) > This bug still affects Mac Safari as of version 5.0.5 (6533.21.1, r86784). > But I don't see any changes in this patch for the Mac port. yeah I already noticed it. The latest patch fixes Mac too (In reply to comment #14) > (In reply to comment #13) > > thanks for looking over this. > > > Can you elaborate on why we can't test this? It seems like we could at least > > have a manual test case. > > Yeah this is pretty easily testable. The test case is actually available in this bug. I thought only automated test cases can be mentioned. Will fix it. The attachment 43844 [details] can definitely be automated by eventSender. Created attachment 94002 [details]
trivial patch
now added Mac for real, elaborated changelog.
Comment on attachment 94002 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=94002&action=review > Source/WebCore/ChangeLog:7 > + The isolated testcase can be found at the bug: > + https://bugs.webkit.org/show_bug.cgi?id=31721#c5 This is insufficient. The test case should be included in the patch. As mentioned by Ryosuke Niwa, we can write a test case for this bug using DRT and the EventSender object. One such example that uses EventSender to click a context menu item is <http://trac.webkit.org/browser/trunk/LayoutTests/media/context-menu-actions.html>. > Source/WebCore/ChangeLog:189 > +>>>>>>> .r86799 Merge conflict marker. Comment on attachment 94002 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=94002&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:297 > ASSERT(cocoaURL); Wouldn't cocoaURL be nil here since Editor::copyImage() would pass an empty KURL()? Hence, an assertion failure. Comment on attachment 94002 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=94002&action=review >> Source/WebCore/platform/mac/PasteboardMac.mm:297 >> ASSERT(cocoaURL); > > Wouldn't cocoaURL be nil here since Editor::copyImage() would pass an empty KURL()? Hence, an assertion failure. hm, looks like you are right. I am reversing the order so that this does not happen Created attachment 94169 [details]
new patch
new patch.
this time with a brand new layout test.
Comment on attachment 94169 [details] new patch Attachment 94169 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8721200 New failing tests: editing/pasteboard/copy-standalone-image-crash.html Created attachment 94171 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 94174 [details]
new patch
this time wont break cr-linux hopefully
Comment on attachment 94174 [details]
new patch
looks like bots are happy. Asking for review
Comment on attachment 94174 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=94174&action=review > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:8 > + layoutTestController.dumpEditingCallbacks(); What's the point of dumpEditingCallbacks if you're just going to overwrite it all with PASS in the end? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 > + setTimeout(doctxmenu, 10); > + setTimeout(hideframe, 3000); Do we need to use setTimeout? That makes tests slow and flaky. Are there some events we should wait for instead? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:65 > + layoutTestController.waitUntilDone(); This should be in the if-block at the top of the page. Comment on attachment 94174 [details]
new patch
The code change looks good, but the test probably needs another iteration.
Comment on attachment 94174 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=94174&action=review >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:8 >> + layoutTestController.dumpEditingCallbacks(); > > What's the point of dumpEditingCallbacks if you're just going to overwrite it all with PASS in the end? agreed, this is pretty dumb >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 >> + setTimeout(hideframe, 3000); > > Do we need to use setTimeout? That makes tests slow and flaky. Are there some events we should wait for instead? the bigger 3s timeout is for manual testing. For automation at least one timeout is needed. Anyway I reduced amount of timeouts and now with the automation there is only on 10ms timeout. I hope that is good. The tests executes at 0.4s on my box Created attachment 94308 [details]
new version
Reduced timeouts usage in automated script, now gross exec time is down to 0.4s.
Fixed other suggestions.
Comment on attachment 94308 [details] new version View in context: https://bugs.webkit.org/attachment.cgi?id=94308&action=review I'm sorry but you'd have to polish up your test. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:1 > +<html> Missing <!DOCTYPE html> > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:12 > +function doclick() { camelCase. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:13 > + Why does the every function start with a blank line? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:23 > + // it is pretty tricky to deal with accelerators in a decent way, but I suppose > + // just removing the underscores is a good enough way > + I don't think this comment adds any useful information. Please remove it. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:32 > + // As long as didn't crash, we passed. This comment is redundant. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:38 > +function hidediv() { camelCase. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:41 > + var div = document.getElementById ("DIV"); > + div.style.display="none"; No need for a temporary div. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:56 > + if (!window.layoutTestController) > + return; > + Wrong indentation level. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 > + items = eventSender.contextClick(); What's the point in calling contextClick() here? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:67 > + setTimeout(doclick, 10); What's the point of this setTimeout? Comment on attachment 94308 [details] new version View in context: https://bugs.webkit.org/attachment.cgi?id=94308&action=review >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:32 >> + // As long as didn't crash, we passed. > > This comment is redundant. copied it from some other tests (found it useful actually), but sure >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 >> + items = eventSender.contextClick(); > > What's the point in calling contextClick() here? to popup context menu and to programmatically learn action items available >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:67 >> + setTimeout(doclick, 10); > > What's the point of this setTimeout? to execute detachment of render to the node WHILE the context menu is popped up. This is the heart of the problem in this bug Created attachment 94317 [details]
next version
the new version hopefully addresses Ryosuke's comments.
Comment on attachment 94317 [details]
next version
forgot doctype
Created attachment 94319 [details]
new version
the once again new version.
if I polish it more I probably would tear the paint ;)
Comment on attachment 94319 [details]
new version
Why not add this guard to caller side? Pasteboard method already has so many code duplication between ports.
I don't want more.
(In reply to comment #35) > (From update of attachment 94319 [details]) > Why not add this guard to caller side? Pasteboard method already has so many code duplication between ports. > I don't want more. yeah, but code duplication in pasteboard is unavoidable. If it is designed that way we just have to live with it or change design (In reply to comment #36) > (In reply to comment #35) > > (From update of attachment 94319 [details] [details]) > > Why not add this guard to caller side? Pasteboard method already has so many code duplication between ports. > > I don't want more. > > yeah, but code duplication in pasteboard is unavoidable. If it is designed that way we just have to live with it or change design Did you miss an assertion on the Gtk port for: ASSERT(node->renderer()->isImage()); From that code alone, it appears so. This is why i like the above comment from Morita. Pasteboard needs some refactoring and this bug may be a good start in this direction. Mihnea (In reply to comment #37) > Did you miss an assertion on the Gtk port for: > ASSERT(node->renderer()->isImage()); > > From that code alone, it appears so. This is why i like the above comment from Morita. Pasteboard needs some refactoring and this bug may be a good start in this direction. No, I did not miss it. Some platform code asserts isImage(), some doesn't. I thought the idea of this bug was just to fix a crash. Refactoring should be done under different bugs in my opinion. Now then, shall I just move the check to the caller? Comment on attachment 94319 [details]
new version
Andrey's claim makes sense: The bug fix and refactoring should have separate patches. So let's fix it, and start refactoring!
The commit-queue encountered the following flaky tests while processing attachment 94319 [details]: animations/suspend-resume-animation.html bug 48161 (authors: cmarrin@apple.com and simon.fraser@apple.com) The commit-queue is continuing to process your patch. Comment on attachment 94319 [details] new version Rejecting attachment 94319 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1 Last 500 characters of output: ource/JavaScriptCore/assembler/AbstractMacroAssembler.h M Source/JavaScriptCore/assembler/LinkBuffer.h M Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h M Source/JavaScriptCore/ChangeLog r87356 = 0d15b9e08d45889b062418435b04bb482fb278a8 (refs/remotes/trunk) M LayoutTests/platform/qt/Skipped M LayoutTests/ChangeLog r87357 = 222d6ccfdb66b9a5e75f5b5d5a1ef79f89c91769 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8733800 Morrita, thanks. Looks like you need to modify changelog: ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Comment on attachment 94319 [details]
new version
If the renderer can be gone, then I expect it can also be a non-image renderer due to changes in the CSS content attribute. So I think that other assertion, the one about isImage, is also incorrect.
(In reply to comment #43) > (From update of attachment 94319 [details]) > If the renderer can be gone, then I expect it can also be a non-image renderer due to changes in the CSS content attribute. So I think that other assertion, the one about isImage, is also incorrect. Darin, do you think you can hack together a simple javascript/css testcase? CC'ing Darin Adler since Andrey Petrov responded to him in comment 44. No, sorry, I don’t have time to work on this personally to test my guess at the moment due to my work load. Ok thanks. Then we can a file new bug later for this if we do run into a case when it triggers. Perhaps this can be moved to caller as well when we do the refactoring already mentioned. (In reply to comment #47) > Then we can a file new bug later for this if we do run into a case when it triggers. Or we can make code that doesn’t just assert something because it wishes for it. It’s best to have reproducible cases for bugs to fix, but we also can write code that doesn’t make assertions for things that can’t be guaranteed. So do you Darin think I should nuke the isImage() asserts then? (I am not expert in this and I a bit cautious about making changes which I can not reproduce) (In reply to comment #49) > So do you Darin think I should nuke the isImage() asserts then? No, I think we should change the isImage asserts into actual isImage runtime checks. Created attachment 95192 [details]
new version
new version also adds isImage() runtime check as suggested
Comment on attachment 95192 [details]
new version
bots seem to be happy. asking for review
Comment on attachment 95192 [details]
new version
let us try again.
Comment on attachment 95192 [details] new version Clearing flags on attachment: 95192 Committed r87709: <http://trac.webkit.org/changeset/87709> All reviewed patches have been landed. Closing bug. The test is failing on Windows port: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r87953%20(13385)/editing/pasteboard/copy-standalone-image-crash-pretty-diff.html Should it be skipped? hm looks like this is a limitation of eventsender, the following code does not seem to work on windows: actionitems = eventSender.contextClick(); So EventSender is unable learn context menu items. Ryosuke do you know if this is a limitation on windows of eventsender? anyway, what we can do on windows is to abort the test and consider it passed if we learn there is nothing coming from contextClick(). Since we abort the test technically it is a lie and it did not 'pass' as such. Is it a better option than to skip the test on windows? (In reply to comment #58) > anyway, what we can do on windows is to abort the test and consider it passed if we learn there is nothing coming from contextClick(). Since we abort the test technically it is a lie and it did not 'pass' as such. > > Is it a better option than to skip the test on windows? We normally add failing test expectation and file a bug. +jberlin & +aroben although aroben seems to be on vacation now. (In reply to comment #59) > (In reply to comment #58) > > anyway, what we can do on windows is to abort the test and consider it passed if we learn there is nothing coming from contextClick(). Since we abort the test technically it is a lie and it did not 'pass' as such. > > > > Is it a better option than to skip the test on windows? > > We normally add failing test expectation and file a bug. +jberlin & +aroben although aroben seems to be on vacation now. Adam is on vacation for the next couple of weeks. Technically I am on vacation today and Monday, but I can’t resist the urge to check my email ;) In a case like this, where the cause of the failure is that we do not have a DRT / WKTR implementation, we usually add the test to the Skipped list. You should add it under the section for contextClick: # Need to dump context menu items on eventSender.contextClick(true). # https://bugs.webkit.org/show_bug.cgi?id=39104 editing/spelling/context-menu-suggestions.html I did this for mac-wk2 yesterday because WKTR does not have an implementation of eventSender.contextClick. |