Plugins stop working after first clicking on reload once

VERIFIED FIXED in mozilla0.9.3

Status

()

Core
Plug-ins
P1
blocker
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Peter Lubczynski, Assigned: Peter Lubczynski)

Tracking

({regression})

Trunk
mozilla0.9.3
x86
Windows 2000
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [baking in trunk], pdt+, URL)

Attachments

(4 attachments)

(Assignee)

Description

17 years ago
Simple Testcase:
http://slip.mcom.com/shrir/flashedit.html

1) Go to the simple testcase (build 622 pre-branching) or any other
2) Click Reload.
3) Notice plugin is missing

Clicking reload again brings the plugin back.
(Assignee)

Updated

17 years ago
Blocks: 85334
(Assignee)

Comment 1

17 years ago
It seems that when I hit Reload(), we never get any
nsPluginStreamListenerPeer::OnDataAvailable() calls even though a Start and Stop
are issued. So it works on alternating reloads, how weird!

cc:ing DougT and Darin if they know of any known issues with Reload()?
(Assignee)

Comment 2

17 years ago
More to add: so the current pattern goes like....

shows >> reload >> BLANK >> reload >> shows >> realod >> BLANK >> reload >> shows


Adding a twist by clicking the "Clear Disk Cache" under the Advance Cache Prefs
in between this cycles seems to FIX the problem. This is weird:

shows >> reload >> BLANK >> reload >> shows >> CLEAR DISK CACHE >> shows >>
reload >> BLANK

(Assignee)

Comment 3

17 years ago
...my mistake, there is a reload after CLEAR DISK CACHE..... which shows
peforming this action before clicking reload prevents the next page from coming
up blank. Without doing it each time, it comes up blank. 


shows >> reload >> BLANK >> reload >> shows >> CLEAR DISK CACHE >> reload >>
shows >> reload >> BLANK


Comment 4

17 years ago
I've been seeing this for some time now. It always seemed a bit more random than
what you describe but, hey, I was trying to work around it... :) I thought it
was an updating issue, not a loading issue, but you have obviously spent more
time researching it than I did.

Comment 5

17 years ago
the OnStopRequest is returning a bad status.  Digging a bit deeper, the cache 
file that http is reading from does not exist.  

I think that this cache file was not closed/flushed from the prior load of the 
plugin.  Or, does the plugin code delete this cache file out from under the 
cache?  How is this case handled? Gordon, do you stat the file when someone asks 
that a disk entry be valid? (probably too expensive to do this).
(Assignee)

Comment 6

17 years ago
Hm.... I comented out some file deleteing code in plugins and that didn't help
but I may have missed something. I'll go back and check plugin code.

Comment 7

17 years ago
peter: you might try inspecting about:cache to see if the cache entry exists...
though the data file may not be there.
(Assignee)

Comment 8

17 years ago
Darin,

I opened up two Navigator windows, one with about:cache (expanding the disk
cache) and the other with my simple testcase:
http://panther.mcom.com/testcases/plugins/testcases/flash3.html

Results:
When the plugin is visible and correctly shown, it shows up in disk cache list.
When the plugin is missing and blank, it does not.

Question:
Are we doing something wrong on cleanup? nsICachingChannel->SetCacheAsFile(1)
returns NS_OK in nsIStreamListener->OnStartRequest.
(Assignee)

Comment 9

17 years ago
I think I may have found the problem (investigating).....this piece of code:

  // If we are writting the stream to disk ourselves, lets close it
  nsCOMPtr<nsIOutputStream> outStream;
  mPluginStreamInfo->GetLocalCachedFileStream(getter_AddRefs(outStream));
  if (outStream) {
     outStream->Close();
  }

....in OnStopRequest never gets called, even on good streams, because
GetLocalCacheFileStream seems to check for null and returns.

Comment 10

17 years ago
ah, so if the file isn't closed, then you won't be able to open it again.
(Assignee)

Comment 11

17 years ago
I double checked that code, we aren't calling it. I was barking up the wrong
tree there.

Taking a closer look at nsHttpChannel::Connect(), I think finally found a branch
in the code path. On reloading when it becomes visible, we fall through the
bottom. If this is a alternate reload, we return ReadFromCache().

Since now I think the cache is suspect, I'll try experimenting with different
load flags bug I'm starting to think this is a Necko problem. This used to work.
Status: NEW → ASSIGNED
Keywords: nsBranch, regression
Priority: -- → P1
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 12

17 years ago
Created attachment 40080 [details] [diff] [review]
simple patch to bypass cache....fixes this bug!
(Assignee)

Comment 13

17 years ago
Sorry for the gibberish above, it?s hard to type and talk on the phone. Anyway,
my patch fixes this bug by always bypassing the cache. Is that something we
really want to do for plugins? 

Comment 14

17 years ago
If you bypass the cache, then all of the file downloading will happen in the 
plugin code, just as if the load came from an https site.  This also means that 
page transitions will cause an entire reload of the plugin.
(Assignee)

Comment 15

17 years ago
Yeah, I know, there may be other risky side effects from doing this as well. I'd
much rather use the Necko cache, like design. I'll keep looking through the
nsHttpChannel source in the debugger but I'd be great if someone more familiar
with the Necko could take a closer look.
(Assignee)

Comment 16

17 years ago
Ahh, I found the problem. Gagan, thanks for your help! It was introduce in my
last check-in to remove temporary files for ByteRanges and https.

Attaching patch that I think will fix this, the right way.

The problem was that when a file stream finished, the actual file in the cache
was being deleted from plugin code causing the cache to become miss-matched. So
next time we loaded that stream, the cache returned an error file not found when
it wasn't there.

Doug, you wrote most of that https file caching code, is it safe NOT to delete
the "cache" file? I thought we kept those files sperate. I think this is the
right thing to do, can I get a review if you agree?
Keywords: patch
Whiteboard: [seeking reviews]
(Assignee)

Comment 17

17 years ago
Created attachment 40121 [details] [diff] [review]
patch to remove deleting of cache files in plugin code

Comment 18

17 years ago
there is NEVER a case when it is okay for you to delete a cache file.  if you 
wish to remove a cache file, then you must Doom the cache entry.
(Assignee)

Comment 19

17 years ago
Attaching new patch to be a little safer with https streams. That code is meant
to delete https plugin locally cached files. After talking with DougT, simply
removing it would be bad. So, instead, I've made it a little smarter by adding a
mLocallyCached boolean and only deletes the file if plugin code is doing the
caching.

Once again, seeking reviews.
(Assignee)

Comment 20

17 years ago
Created attachment 40132 [details] [diff] [review]
new patch to safely remove https cached files

Comment 21

17 years ago
r/sr=darin

(Assignee)

Updated

17 years ago
Blocks: 76892

Comment 22

17 years ago
sr|r

Comment 23

17 years ago
You are going to leak mFilePath if mLocallyCached is false.

Also, tab/space issues all over the place... don't mess with the whole file, but
could you please clean up in the areas your patch touches?
(Assignee)

Comment 24

17 years ago
Created attachment 40162 [details] [diff] [review]
patch to fix leaking string and tab fix-up
(Assignee)

Comment 25

17 years ago
Okay, this patch is in the trunk for baking. On to bug 85334.
Whiteboard: [seeking reviews] → [baking in trunk]

Comment 26

17 years ago
marking pdt+ since bug 85334 depends on this also, and that's a pdt+ bug. 
CC'ing chofmann -- it's been "nsBranch"d also, but not by us :-)
Whiteboard: [baking in trunk] → [baking in trunk], pdt+

Comment 27

17 years ago
pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
(Assignee)

Comment 28

17 years ago
Patch now in branch, marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
*** Bug 88636 has been marked as a duplicate of this bug. ***

Comment 30

17 years ago
*** Bug 88564 has been marked as a duplicate of this bug. ***

Comment 31

17 years ago
yeha, verified on truk and branch .working fine now. no problems seen 0705  
builds winNT
Status: RESOLVED → VERIFIED

Comment 32

17 years ago
*** Bug 89845 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.