Simple test case: paste this URL into your browser javascript:document.open();document.write("<body>test</body>");document.close() In IE and FF, a new page is created, and you can click Back to go to the page you were on before. In Safari, no back/forward item is created. Instead, the current page is temporarily replaced (you can get back to it by navigating elsewhere, going back, then reloading). This is not a regression, as it happens in shipping Safari on OS X (10.4.10).
I'm going to try working on this. Here's my idea so far, so maybe someone can tell me whether it sounds reasonable or if I am way off base: 1. Route the document.write() text through the FrameLoader using SubstituteData, so the normal load events get fired. - document.open will start a load with an empty SubstituteData - document.write will append to the SubstituteData, which will write to the tokenizer - document.close will finish the load This part will require some changes to the way SubstituteDate can be loaded (right now it must come in a single chunk). 2. Store SubstituteData on the HistoryItem, so the page can be restored when you navigate back to it.
since document.open came up on IRC yesterday, might wanna take a look at bug #14959
Eh correction #14968 of course.
Created attachment 16045 [details] work in progress Here's what I have so far, in case anyone cares. It's certainly not finished, as there are some bugs, failing tests, and there's no new tests included. But the basic idea is there (which is generally to have document.write go through the Frameloader instead of writing directly to the tokenizer). If anyone cares to take a look, any suggestions would be appreciated. One issue I haven't figured out how to solve yet is that my call to FrameLoader::load in Document::open results in the document being closed and reopened again. It also doesn't work very nicely with the page cache, which I've turned off so far.
After struggling to get my original approach to work, I've decided to go another route. Using the FrameLoader code caused too many problems. Now I'm going to attempt something simpler: 1. In document.open(), add a new HistoryItem that holds an empty SubstituteData. 2. In document.write(), append to the SubstituteData. 3. When navigating to a HistoryItem with a valid SubstituteData, load that data as if it were the page contents.
Created attachment 16160 [details] proposed patch This patch adds support for putting document.open() generated pages in session history. javascript: urls should also create new history items, but I'll attack that in another patch (should be much simpler).
Just commenting on this bug so it doesn't get lost.
Don't worry, it's not lost. We are trying to get through the review queue which sadly had a bit of a backlog.
Created attachment 18052 [details] updated patch Fixes to previous patch to make it apply and build again.
Comment on attachment 18052 [details] updated patch Patch looks great. I have a few complaints that should be relatively simple to fix. Many apologies for not reviewing this long ago. I had assumed that someone else was going to review. Thank you very much for working on this. 1311 // Params set to match the old behavior, though replace=false is probably a more sane default. 1312 open("text/html", true); I don't think the issue here is "sane default" but rather matching other browsers, since open is a public DOM function accessible from JavaScript. Can this change to replace = false? If not, then why not? This comment makes it seem like something is broken. Is something broken here? If so, what? What test would demonstrate the breakage? 113 String mimeType("text/html"); 114 if (args.size() >= 1) { 115 mimeType = String(args[0]->toString(exec)).lower(); 116 if (mimeType != "text/html" && mimeType != "replace") 117 mimeType = "text/plain"; // anything other than text/html is treated as plaintext. 118 } It's unnecessarily inefficient to actually allocate the buffer for the string "text/html" -- the logic should be changed so that it gets set on both sides of an if. Generally, we do not ever check the size of arguments. Instead we rely on the fact that arguments not passed are undefined. There are very few counterexamples to this (although one of them is earlier in this very function). And the JavaScript arguments list object makes this easy by returning undefined for any values past the end of the list. So this code should check args[0]->isUndefined() rather than args.size(). It's also worth testing behavior of other browsers when the value null is passed in -- is this treated as the MIME type "text/plain" or as "text/html"? It would be good to have a few tests that cover this. The comment doesn't make it clear at all why the value "replace" is allowed for mimeType and I must admit I don't understand why. The comment should start with a capital letter if it ends with a period. 120 bool replace = false; 121 if (args.size() >= 2) 122 replace = equalIgnoringCase("replace", String(args[1]->toString(exec))); The best way to handle this is probably to just leave out the if statement. We can let values like undefined get converted to string and rejected since they aren't equal to "replace". 920 // Contains the text written to the document by script, eg through document.write(). 921 RefPtr<SharedBuffer> m_scriptedDocumentBuf; How about a name that matches the comment more closely? Perhaps m_textWrittenByScript. I don't like the existing name because we don't normally abbreviate buffer as "buf" and I don't see how "scripted document buffer" makes sense, since there is no such thing as a scripted document. 1578 open(String("text/html"), false); Why the explicit cast to String here? 1567 write(DeprecatedString("<html>")); 1582 write(String("<html>")); Can we get rid of the DeprecatedString overload? 1587 if (m_scriptedDocumentBuf) { 1588 CString utf8 = text.utf8(); 1589 m_scriptedDocumentBuf->append(utf8.data(), utf8.length()); 1590 } Is there a good reason to convert this text to UTF-8 then decode it back to UTF-16 later? We really don't want to be converting back and forth between UTF-16 and UTF-8 unless there's a benefit to doing so. I'd like to avoid the extra memory allocation too. 350 void open(const String&, bool); While we do omit argument names in headers when they are not needed, this is a case where they are needed. It's not at all clear what the string or boolean are. We leave the names out when the argument type or function name makes the name superfluous. 740 bool newItem = false; The boolean should not be named "new item" because it is a boolean flag, not an item. It should be named something like isItemNew, itemIsNew or historyItemIsNew. 755 DeprecatedString generatedURLString("generated:"); 756 generatedURLString.append(m_frame->document()->url()); 757 KURL generatedURL(generatedURLString); For code like this, it's generally better to not make a temporary -- for one thing it means we have to revisit this callsite when eliminating DeprecatedString. This should simply be: KURL generatedURL("generated:" + m_frame->document()->url()); Perhaps the URL scheme should also be changed to something with a vendor prefix. Just plain "generated:" seems like it might conflict with other uses. Something with webkit in it would probably be better. Can the URL used here be detected by JavaScript? If so, what effect will the URL have on site compatibility? What URL do you see on other web browsers in this case? Do any of the tests detect this generated URL? 759 SubstituteData substituteData(buffer, mimeType, String("UTF-8"), m_URL, generatedURL); 760 item->setSubstituteData(substituteData); This should use UTF-16 and we should not be converting text to UTF-8 at all. And I think there's no need to have a named temporary here. 20822108 void FrameLoader::load(const ResourceRequest& request, const NavigationAction& action, FrameLoadType type, PassRefPtr<FormState> formState) 2113 void FrameLoader::load(const ResourceRequest& request, const NavigationAction& action, FrameLoadType type, PassRefPtr<FormState> formState, const SubstituteData& substituteData) We're really concerned about the huge number of load functions in the FrameLoader class and we're trying to fix that. Adding yet another is unfortunate. On the other hand, since this effectively just gives the last parameter a default value of null, I think it's OK.
Created attachment 18369 [details] revised patch
(In reply to comment #10) > (From update of attachment 18052 [details] [edit]) > Patch looks great. > > I have a few complaints that should be relatively simple to fix. > > Many apologies for not reviewing this long ago. I had assumed that someone else > was going to review. Thank you very much for working on this. Thanks for taking the time to do a thorough review. > 1311 // Params set to match the old behavior, though replace=false is > probably a more sane default. > 1312 open("text/html", true); > > I don't think the issue here is "sane default" but rather matching other > browsers, since open is a public DOM function accessible from JavaScript. Can > this change to replace = false? If not, then why not? This comment makes it > seem like something is broken. Is something broken here? If so, what? What test > would demonstrate the breakage? I revised the comment, which I think was misleading. Document::open() is not called from JavaScript (the one with params is). open() is called in various places internal to WebCore, and I just wanted to preserve the old calling defaults to avoid unintentionally changing anything. An example callsite is DOMImplementation::createHTMLDocument(). > KURL generatedURL("generated:" + m_frame->document()->url()); > > Perhaps the URL scheme should also be changed to something with a vendor > prefix. Just plain "generated:" seems like it might conflict with other uses. > Something with webkit in it would probably be better. Can the URL used here be > detected by JavaScript? If so, what effect will the URL have on site > compatibility? What URL do you see on other web browsers in this case? Do any > of the tests detect this generated URL? I went with "webkitgenerated". As far as I can tell, this url (used for the substitute data's responseURL) is entirely internal to WebCore, and not visible to the user or JavaScript at all. The main reason in making it different is so it looks like a different page to the FrameLoader, and we don't try to do a reload. Firefox uses "wyciwyg:" (what you cache is what you get). I believe I took care of all your other suggestions.
Comment on attachment 18369 [details] revised patch Looks great Matt. I'll leave this for Darin to review for real though. I should note, there are two single line ifs which differ from the style guidelines: } else { 120 mimeType = "text/html"; 121 } 9 if (m_textWrittenByScript) { 1590 m_textWrittenByScript->append(reinterpret_cast<const char*>(text.characters()), 1591 text.length() * sizeof(UChar)); 1592 } Otherwise the patch as a whole looks sane. I'll leave darin to do the final r+.
Comment on attachment 18369 [details] revised patch It would be good to make this do something predictable when toString raises an exception, but it's not like we're doing that well elsewhere. r=me
I applied the patch and ran tests. And all the new tests failed.
Comment on attachment 18369 [details] revised patch The patch is great, but the file document-open.js is missing from the tests directory. We need a version of the patch that includes that new file.
Created attachment 18447 [details] revised patch (fixed) Oops, sorry about that. Resource files added.
Comment on attachment 18447 [details] revised patch (fixed) OK. I'll try to land this next time I have some free time.
Committed revision 29816.
This seems to have broken back/forward on Gmail. 'I can't navigate to Gmail's inbox page from Gmail Compose's page. When I login into my gmail account and click "Compose Mail", the url shows <http://mail.google.com/mail/#compose>. Clicking the back toolbar icon, should go to <http://mail.google.com/mail/#inbox> but the page redirects and remains at <http://mail.google.com/mail/?ui=2&view=bsp&ver=ymdfwq781tpu>". Given that, I think we're going to have to roll this change out.
Unfortunately, this patch broke Gmail! Rolling it out and re-opening the bug.
Another case that may be easier to reduce was found here: * STEPS TO REPRODUCE 1. open http://www.greatschools.net/content/schoolChoiceCenter.page 2. Click any article, but not the menu (ex. Choosing a School:) 3. Click Back button till you get to original page. * RESULTS I get a page with a picture link, then another page with another link before finally coming back to original fully displayed page.
Created attachment 18860 [details] reduced test case Thanks for the alternate repro case, Kevin. I was able to make a reduced test case from it. The problem is from the way ads are added to the page: by write()ing to an iframe's contentWindow.document.
Comment on attachment 18447 [details] revised patch (fixed) Clearing the review flag to get this out of commit queue.
Can someone please update this ticket ? The issue seems to be forgotten event though it is apparently fixed if someone would spend the required time on getting the patch in the tree.
Sorry, I sort of dropped the ball on this. My initial patch caused some regressions with Gmail and other sites. I then got distracted by other things and haven't had time to work on it since. I don't know when I'm going to have time to fix the issues with my patch.
Created attachment 134004 [details] Patch
Added a patch for the issue, using Matt Perry's patch. The new patch has the fix for document.write in a iFrame , i.e fixes the navigation in Gmail (Comment #20 by From Darin Adler).
Comment on attachment 134004 [details] Patch This patch contains numerous style violations and lacks any tests. If you're interested in contributing to WebKit, you might want to start with an easier bug. This one pretty tricky.
Comment on attachment 134004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134004&action=review > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:128 > + // See bug <https://bugs.webkit.org/show_bug.cgi?id=14959> for discussion. We don't usually put comments like this in the source files. We use "svn annotate" and ChangeLogs to find the bug threads about the code. > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:129 > + UString firstString = exec->argument(0).toString(exec)->value(exec); What if toString throws an exception? > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:132 > + if (mimeType != "text/plain") This check doesn't seem like it can be right. What if the mimeType is "text/plain; charset=utf-8" ? > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:133 > + mimeType = "text/html"; This indent is wrong. > Source/WebCore/dom/Document.cpp:2125 > + // This method is called by various places in the WebCore code. Arguments > + // chosen for legacy reasons. This comment is pretty vague. Why else would a function exist other than to be called from various places? > Source/WebCore/dom/Document.cpp:2163 > if (m_frame) > - m_frame->loader()->didExplicitOpen(); > + m_textWrittenByScript = SharedBuffer::create(); // Buffer to record data written by document.write calls. > + m_frame->loader()->didExplicitOpen(mimeType, replace, m_textWrittenByScript.get()); Did you mean to put { } around the body of this if-statement or did you mean for the second line to be executed unconditionally?
Comment on attachment 134004 [details] Patch The patch also needs to include a test case showing the bug is fixed.
Created attachment 135517 [details] Patch
Created attachment 135522 [details] Patch
Have added a revised patch with test case.
Created attachment 135548 [details] Proposed Patch
Created attachment 135549 [details] TestCases_Patch
(In reply to comment #30) > > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:129 > > + UString firstString = exec->argument(0).toString(exec)->value(exec); > > What if toString throws an exception? Have added check to test if "argument(0)" is Undefined or not. > > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:132 > > + if (mimeType != "text/plain") > > This check doesn't seem like it can be right. What if the mimeType is "text/plain; charset=utf-8" ? mime type is extracted by checking if the type string contains the "MIMETYPE" in a case insensitive way. Also changed the structure/functions from the previous patch. Now there is one Document::open(mime, replace, ownerDoc) which is called from JSHTMLDocument instead of the regular Document::open(ownerDoc). Document::open(mime, replace, ownerDoc) in turn calls the Document::open(ownerDoc) and also handles adding/replacing history item contents through FrameLoader. Document::open(mime, replace, ownerDoc) is also responsible in deciding if a history item must be modified/added for that particular frame. Moved the actual History Item manipulation from FrameLoader to HistoryController class PS: I have 2 seperate patches, one for the code changes and one for the test cases. EWS was/is failing for all platforms and style. So thought adding code changes and Test cases could help... Maybe not so !!
Comment on attachment 135549 [details] TestCases_Patch Attachment 135549 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12325810 New failing tests: http/tests/navigation/document-open-html-adds-history-item.html http/tests/navigation/document-open-default-new-window-adds-history-item.html http/tests/navigation/document-open-html-replace-no-history-item.html http/tests/navigation/document-open-default-adds-history-item.html http/tests/navigation/document-write-without-open-adds-history-item.html http/tests/navigation/document-open-in-iframe-no-history.html
Created attachment 135591 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
The code change and test cases need to be in the same patch.
Ok, i'll upload another patch with both code changes and test cases.
Created attachment 136129 [details] Proposed Patch
Comment on attachment 136129 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136129&action=review This patch has many serious problems. As I wrote in Comment #29: If you're interested in contributing to WebKit, you might want to start with an easier bug. This one pretty tricky. > Source/WebCore/loader/HistoryController.cpp:890 > + KURL generatedURL(ParsedURLString, "webkitgenerated:" + url); What is the "webkitgenerated" scheme? That looks like something you just made up.
This bug has too much noise in it to be useful for actually solving this problem.
> This patch has many serious problems. As I wrote in Comment #29: If you're interested in contributing to WebKit, you might want to start with an easier bug. This one pretty tricky. > > > Source/WebCore/loader/HistoryController.cpp:890 > > + KURL generatedURL(ParsedURLString, "webkitgenerated:" + url); > > What is the "webkitgenerated" scheme? That looks like something you just made up. Had to use a different URL for the substitute data. Appended it with webkitgenerated. Was just trying to generate a different URL, from the given one...