| Summary: | Crash in DocumentWriter::endIfNotLoadingMainResource | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ap, japhet, ossy, rniwa, webkit.review.bot, wez, zmo | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adam Barth
2011-08-02 16:42:44 PDT
Created attachment 102714 [details]
Patch
Comment on attachment 102714 [details]
Patch
Can you run that test case on a computer without Flash installed?
> Can you run that test case on a computer without Flash installed?
Let me try with the test plugin.
Created attachment 102721 [details]
Patch
Comment on attachment 102721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102721&action=review > Source/WebCore/ChangeLog:10 > + This function is poorly designed because isLoadingMainResource is a > + poor proxy for determing whether to flush/finish the parser. Really, > + we should how loads complete to match the model in HTML5, but that's Typo: I think you're missing a word, "we should change how loads..."? Also, FIXME in endIfNotLoadingMainResource to this effect? > LayoutTests/fast/loader/reload-zero-byte-plugin.html:16 > + }, 100); > +}, 100); Can we do this without setTimeout, or with shorter timeouts? 200ms isn't too bad, but I feel honor-bound to ask. Comment on attachment 102721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102721&action=review >> LayoutTests/fast/loader/reload-zero-byte-plugin.html:16 >> +}, 100); > > Can we do this without setTimeout, or with shorter timeouts? 200ms isn't too bad, but I feel honor-bound to ask. The underlying problem is that opening content in a new window doesn't generate any sort of events. Normally we could load some HTML that sent use a postMessage, but in this case we need to load this empty plugin. It's possible we could teach the test plugin to send us a message on load... (In reply to comment #6) > (From update of attachment 102721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102721&action=review > > >> LayoutTests/fast/loader/reload-zero-byte-plugin.html:16 > >> +}, 100); > > > > Can we do this without setTimeout, or with shorter timeouts? 200ms isn't too bad, but I feel honor-bound to ask. > > The underlying problem is that opening content in a new window doesn't generate any sort of events. Normally we could load some HTML that sent use a postMessage, but in this case we need to load this empty plugin. It's possible we could teach the test plugin to send us a message on load... Eh, I won't ask you to do that. the test plugin has way too much bolted onto it already imo. When I run this test with r92135 on Mac, I don't get any crash, but get empty test results. That's surprising. run-webkit-tests fast/loader/reload-zero-byte-plugin.html --repeat 100 > but get empty test results
Strike that - I applied the patch incorrectly. But still no crash.
Perhaps the crash is limited to Chromium? The Gtk folks have also asked for this null-check, but I can't find the bug atm. Created attachment 102793 [details]
Patch for landing
> Strike that - I applied the patch incorrectly. But still no crash.
(I assume you tested in DRT and not just in Safari.)
Comment on attachment 102793 [details] Patch for landing Clearing flags on attachment: 102793 Committed r92298: <http://trac.webkit.org/changeset/92298> All reviewed patches have been landed. Closing bug. Skipped the added fast/loader/reload-zero-byte-plugin.html in r92306 because of fast/loader/repeat-same-document-navigation.html crashing fast/loader/reload-zero-byte-plugin.html started crashing after this patch was landed: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/36101 I added it to the skipped list for Qt. I'm not sure why this bug is open. Is there more to do here? Should we file another issue about the Qt crash? |