|
|
DescriptionMap WebURLRequest::RequestContextPlugin to RESOURCE_TYPE_OBJECT.
The WebRequest API should treat both object requests, and requests made
from those objects (e.g. Flash requesting an .mp4 resource) as type
Object. We used to do this, then I broke it. :( Now I'm fixing it. :)
BUG=410382
R=vabr@chromium.org
Committed: https://crrev.com/c3e50e6354188999da307094c5c037d5933f1bc3
Cr-Commit-Position: refs/heads/master@{#308787}
Patch Set 1 #Patch Set 2 : A TEST! #Patch Set 3 : Not really a PDF. #Patch Set 4 : Failure? #Patch Set 5 : Rebase #Patch Set 6 : Dropping the test. #Messages
Total messages: 36 (10 generated)
mkwst@chromium.org changed reviewers: + jochen@chromium.org
Vaclav, mind taking a look? Jochen, perhaps you could rubberstamp content/?
LGTM, Thanks for fixing this, Mike! One thing that worries me, is that this is actually not the first time we saw this broken (the previous time was http://crbug.com/128347). We should ideally have some tests to guard this, at least along the lines of a browser test with an extension, asking for a .swf resource, with the test server returning that as a flash object, and the extension checking the request type. I am guilty of not having added such a test when I was fixing the previous breakage, so I'm putting creating that test on my TODO list. No need to block this fix by that. Cheers, Vaclav
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/1
On 2014/10/07 07:40:15, vabr (Chromium) wrote: > LGTM, Thanks for fixing this, Mike! > > One thing that worries me, is that this is actually not the first time we saw > this broken (the previous time was http://crbug.com/128347). We should ideally > have some tests to guard this, at least along the lines of a browser test with > an extension, asking for a .swf resource, with the test server returning that as > a flash object, and the extension checking the request type. > > I am guilty of not having added such a test when I was fixing the previous > breakage, so I'm putting creating that test on my TODO list. No need to block > this fix by that. > > Cheers, > Vaclav This patch will (I believe) fix the reports that requests from plugins aren't correctly mapped to OBJECT. It's not clear to me whether this will fix the larger problem that objects themselves aren't being mapped to OBJECT. According to the switch statement here, they ought to be. If this doesn't fix that problem, we'll have to look elsewhere. :)
The CQ bit was unchecked by mkwst@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
how about adding at least a basic test that tries to load a non-existant plugin?
mkwst@chromium.org changed reviewers: + jam@chromium.org
jam@: This test is crashing locally at the NOTREACHED in Instance::DidOpen. Do you have any idea what might be causing that? The comment kinda worries me. :)
jam@: See the trace at http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
maybe try an existing pdf? chrome/test/data/pdf/test.pdf for example
On 2014/10/13 08:12:28, jochen wrote: > maybe try an existing pdf? chrome/test/data/pdf/test.pdf for example It shouldn't effect this particular case, as we're blocking the request. That said, I'll give it a shot. :)
Huh. That worked. I'll file a bug for the crash.
lgtm
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Hrm. This passes locally. :(
maybe you need to add the files to the browser_tests.isolate?
On 2014/10/14 13:34:56, jochen wrote: > maybe you need to add the files to the browser_tests.isolate? browser_tests.isolate includes 'test/data', which should bundle all of this (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser_tes...). :/
On 2014/10/14 13:42:53, Mike West wrote: > On 2014/10/14 13:34:56, jochen wrote: > > maybe you need to add the files to the browser_tests.isolate? > > browser_tests.isolate includes 'test/data', which should bundle all of this That said, you're right: it fails if I run the tests isolated locally. Grrr.
On 2014/10/14 13:56:12, Mike West wrote: > On 2014/10/14 13:42:53, Mike West wrote: > > On 2014/10/14 13:34:56, jochen wrote: > > > maybe you need to add the files to the browser_tests.isolate? > > > > browser_tests.isolate includes 'test/data', which should bundle all of this > > > That said, you're right: it fails if I run the tests isolated locally. Grrr. I don't see this being related to isolate; it looks like a race condition based on the callstack in http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/580001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Any reason the fix hasn't been pushed through by now? It's fine if the test is going to take longer to get working but I don't see the point in leaving the original bug in place for any longer. As vabr said "No need to block this fix by that."
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/630973002/600001
Message was sent while issue was closed.
Committed patchset #6 (id:600001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c3e50e6354188999da307094c5c037d5933f1bc3 Cr-Commit-Position: refs/heads/master@{#308787}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:600001) has been created in https://codereview.chromium.org/809103003/ by jianli@chromium.org. The reason for reverting is: Speculative revert since it might cause browser test PDFExtensionTest.BasicPlugin to fail. http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20(dbg)/bu... http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/1... http://build.chromium.org/p/chromium.webkit/builders/Win7%20Tests/builds/7530 . |