Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183763030:
Remove DocumentLoader::clearArchiveResources (Closed)
|
Created:
6 years, 9 months ago by ingemara Modified:
6 years, 9 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Daniel Bratell Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionRemove DocumentLoader::clearArchiveResources
It is possible for the DocumentLoader to lose it's ArchiveResourceCollection
without it's MHTMLArchive being deallocated as well, as is observed when a
navigation, which may be aborted, is initiated. By not clearing the
ArchiveResourceCollection in such cases, the DocumentLoader is left in a
still valid state.
BUG=348977
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170148
Patch Set 1 #Patch Set 2 : Add LayoutTest, revert previous patchset, remove clearArchiveResources #Patch Set 3 : _original.html -> .html_original #Patch Set 4 : Don't use local test server for https request #Patch Set 5 : Make sure .mht files are crlf #Patch Set 6 : Trying to get upload.py not strip crlf #Patch Set 7 : Remove LayoutTests #
Messages
Total messages: 28 (0 generated)
Hey, I'm mostly looking for comments on this CL, as removing this assertion doesn't feel like the correct approach. It's put there for a reason, and simply replacing it with a null check might not be correct. I don't know too much about the document loading in Blink to be confident with a change like this. Another alternative might be to /not/ clear m_archiveResourceCollection when navigating away, but this would leave data around for a while longer (which however could be requested, as is the case for 348977) What do you think?
+abarth This seems reasonable to me, but ideally I'd like to find a test that hits this case. Do you have any thoughts about how we can create one?
All I've managed to come up with is a crummy browsertest added to webkit_browsertests.cc, which doesn't really fit the bill. I tried to figure out some way to write a test which hits the assertion purely in Blink, but it wasn't easy, as the DocumentLoader has to be initiated properly with an archive resource through a ResourceFetcher. I'll take a stab at it once more, but if you have any hints I'd appreciate it.
On 2014/03/10 09:57:35, ingemara wrote: > All I've managed to come up with is a crummy browsertest added to > webkit_browsertests.cc, which doesn't really fit the bill. I tried to figure out > some way to write a test which hits the assertion purely in Blink, but it wasn't > easy, as the DocumentLoader has to be initiated properly with an archive > resource through a ResourceFetcher. I'll take a stab at it once more, but if you > have any hints I'd appreciate it. http://src.chromium.org/viewvc/blink/trunk/LayoutTests/mhtml/ has some tests that provide examples of tests with archive loads. If I remember correctly, the one time I created a test like that, I did it by creating a webpage then exporting it as mhtml from a full chromium build.
On 2014/03/10 15:57:40, Nate Chapin wrote: > http://src.chromium.org/viewvc/blink/trunk/LayoutTests/mhtml/ has some tests > that provide examples of tests with archive loads. Oh, you're referring ta a .mht archive which causes Blink to hit the assertion? In that case, I have one uploaded at http://team.opera.com/qa/testbed/wam/WAM-2694/index-ssl.mht. However, I don't think it will be possible to add it as a LayoutTest (or similar), as some interaction is required to reproduce the issue.
On 2014/03/11 07:21:22, ingemara wrote: > On 2014/03/10 15:57:40, Nate Chapin wrote: > > http://src.chromium.org/viewvc/blink/trunk/LayoutTests/mhtml/ has some tests > > that provide examples of tests with archive loads. > > Oh, you're referring ta a .mht archive which causes Blink to hit the assertion? > In that case, I have one uploaded at > http://team.opera.com/qa/testbed/wam/WAM-2694/index-ssl.mht. However, I don't > think it will be possible to add it as a LayoutTest (or similar), as some > interaction is required to reproduce the issue. What kind of interaction is required? Is there a reliable set of steps?
On 2014/03/11 16:06:20, Nate Chapin wrote: > What kind of interaction is required? Is there a reliable set of steps? I can reproduce it reliably through content_shell, desktop/Android Chrome, or through a browser_test which more or less does the same thing as the interaction with content_shell. The manual procedure (interaction steps) is described in http://code.google.com/p/chromium/issues/detail?id=348977. I uploaded a CL to https://codereview.chromium.org/196883002 with the aforementioned browsertest. I'm not intending for it to be integrated to the browser tests in chromium, but rather as an explanation for how I'm reproducing the issue. For brevity I'll explain in more detail what happens, and why the test does what it does: The test simulates a SSL blocking page, from which the user goes "Back to safety" from, by denying requests with a ResourceDispatcherHostDelegate. When a user clicks a link in a frame which has a .mht archive loaded, we're seeing a call stack like: DocumentLoader::clearArchiveResources DocumentLoader::stopLoading FrameLoader::stopAllLoaders FrameLoader::loadWithNavigationAction FrameLoader::load HTMLAnchorElement::handleClick While the test performs a new navigation, the call stack will be similar in the innermost frames. Because of the request being blocked, the document will still be left alive. Since the .mht archive has some css rules embedded which applies to different viewport dimensions, sending a resize message will trigger a resource request. However, as DocumentLoader::clearArchiveResources has been called and the ArchiveResourceCollection is deallocated, the request will hit an assertion/crash.
On 2014/03/12 09:03:42, ingemara wrote: > On 2014/03/11 16:06:20, Nate Chapin wrote: > > What kind of interaction is required? Is there a reliable set of steps? > > I can reproduce it reliably through content_shell, desktop/Android Chrome, or > through a browser_test which more or less does the same thing as the interaction > with content_shell. The manual procedure (interaction steps) is described in > http://code.google.com/p/chromium/issues/detail?id=348977. > > I uploaded a CL to https://codereview.chromium.org/196883002 with the > aforementioned browsertest. I'm not intending for it to be integrated to the > browser tests in chromium, but rather as an explanation for how I'm reproducing > the issue. > > For brevity I'll explain in more detail what happens, and why the test does what > it does: > The test simulates a SSL blocking page, from which the user goes "Back to > safety" from, by denying requests with a ResourceDispatcherHostDelegate. When a > user clicks a link in a frame which has a .mht archive loaded, we're seeing a > call stack like: > > DocumentLoader::clearArchiveResources > DocumentLoader::stopLoading > FrameLoader::stopAllLoaders > FrameLoader::loadWithNavigationAction > FrameLoader::load > HTMLAnchorElement::handleClick You might be able to get in this start from a layout test by trying to navigate to a page that is known not to load? I think layout tests often use "http://localhost:7" for this purpose. > > While the test performs a new navigation, the call stack will be similar in the > innermost frames. Because of the request being blocked, the document will still > be left alive. Since the .mht archive has some css rules embedded which applies > to different viewport dimensions, sending a resize message will trigger a > resource request. However, as DocumentLoader::clearArchiveResources has been > called and the ArchiveResourceCollection is deallocated, the request will hit an > assertion/crash. Hmm, that's interesting. I wonder if the right solution is to wait to call clearArchiveResources() until DocumentLoader::~DocumentLoader(), since it's conceivable that subresource loads will happen after a load is cancelled.
On 2014/03/12 14:43:09, Nate Chapin wrote: > You might be able to get in this start from a layout test by trying to navigate > to a page that is known not to load? I think layout tests often use > "http://localhost:7" for this purpose. Unfortunately, for such an url we'll end up going all the way with the navigation, discarding the document completely. However, I managed to come up with a LayoutTests which successfully reproduces the crash. It's not sophisticated, but works well enough. > > > > While the test performs a new navigation, the call stack will be similar in > the > > innermost frames. Because of the request being blocked, the document will > still > > be left alive. Since the .mht archive has some css rules embedded which > applies > > to different viewport dimensions, sending a resize message will trigger a > > resource request. However, as DocumentLoader::clearArchiveResources has been > > called and the ArchiveResourceCollection is deallocated, the request will hit > an > > assertion/crash. > > Hmm, that's interesting. I wonder if the right solution is to wait to call > clearArchiveResources() until DocumentLoader::~DocumentLoader(), since it's > conceivable that subresource loads will happen after a load is cancelled. I think so. I looked in to the history of the call, and it goes a while back. It's been through a few refactorings, and it's not impossible that it once made sense to clear the resources when all loaders were to stop, but that's no longer the case.
LGTM! Thanks for sticking with this.
The CQ bit was checked by ingemara@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ingemara@opera.com/183763030/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
The CQ bit was checked by ingemara@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ingemara@opera.com/183763030/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
Looks like I made a few mistakes with the layout tests, but they should be fixed now. However, it looks like the changes on rietveld are identical to what's commited by me. I'm thinking of line endings in .mht files, which must be CRLF, but when examining the changes (for instance through git cl patch 183763030), it's evident that they have simply been replaced by '\n'. There were some discussion on https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/WzBiNHj53... The test will never pass as long as the bot fetches the cl from rietveld, since the .mht file will be invalid. I'm not sure how to make rietveld/cl upload not change line endings; I tried editing .gitattributes, but to no avail :(
The CQ bit was checked by ingemara@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ingemara@opera.com/183763030/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
Apparently, https://codereview.chromium.org/161383002 just landed which kills the tests. I don't see any way to reproduce the issues without JS in .mhtml files. I've talked to Nate off-review, and skipping the tests is OK.
The CQ bit was checked by ingemara@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ingemara@opera.com/183763030/100001
Message was sent while issue was closed.
Change committed as 170148
Message was sent while issue was closed.
On 2014/03/27 07:43:25, ingemara wrote: > Apparently, https://codereview.chromium.org/161383002 just landed which kills > the tests. I don't see any way to reproduce the issues without JS in .mhtml > files. I've talked to Nate off-review, and skipping the tests is OK. LGTM and thanks! Sorry for all the wasted extra effort. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
