Last Comment Bug 491134 - nsDOMOfflineResourceList uses its own (unsafe) way to dispatch events
: nsDOMOfflineResourceList uses its own (unsafe) way to dispatch events
: crash, testcase, verified1.9.0.12, verified1.9.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 495764
  Show dependency treegraph
Reported: 2009-05-02 10:48 PDT by Olli Pettay [:smaug]
Modified: 2009-07-21 21:29 PDT (History)
13 users (show)
jst: blocking1.9.1+
dveditz: blocking1.9.0.12+
samuel.sidler+old: wanted1.9.0.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

for trunk (70.26 KB, patch)
2009-05-11 09:10 PDT, Olli Pettay [:smaug]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
For 1.9.1 and 1.9.0 (2.34 KB, patch)
2009-05-14 13:47 PDT, Olli Pettay [:smaug]
jst: review+
jst: superreview+
dveditz: approval1.9.0.12+
Details | Diff | Splinter Review
for trunk (includes backout of the branch patch) (70.78 KB, patch)
2009-06-14 14:01 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2009-05-02 10:48:59 PDT
nsDOMOfflineResourceList pushes context to stack before iterating
event listeners and pops it later. But it does not check if event listener
killed cx or anything like that. I believe this may lead to similar problems 
as what XHR had in Bug 460002.

I'll make nsDOMOfflineResourceList to work like any other event target, but
for branches some other fix is needed.
Comment 1 Olli Pettay [:smaug] 2009-05-11 09:10:07 PDT
Created attachment 376727 [details] [diff] [review]
for trunk

This became quite large, but I really do want all the event dispatching to work the same way. This change allows also simplifications to nsPrivateDOMEvent.

Event handling is moved from nsXHREventTarget to
a new helper class nsDOMEventTargetHelper and static method GetDocumentFromScriptContext from nsXMLHttpRequest.cpp to nsContentUtils.
nsXHREventTarget extends nsDOMEventTargetHelper, and nsDOMOfflineResourceList does that too.
This removes nsDOMOfflineResourceList::NotifyEventListeners, because normal
event handling can be now used with nsDOMOfflineResourceList.

Branches can have simpler change; some kind of CheckInnerWindowCorrectness in
NotifyEventListeners. That is what is done in 1.9.0.x XHR.
Comment 2 Olli Pettay [:smaug] 2009-05-13 03:55:35 PDT
Would be great to have a testcase here. Something like the testcases in
Bug 460002, but using applicationCache and not XHR.

But even without such testcase the patch is needed for trunk.
Comment 3 Daniel Veditz [:dveditz] 2009-05-13 15:51:00 PDT
We may reconsider if the "simpler change" for branches promised in comment 1 ends up not working out.
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-05-13 16:02:02 PDT
I tried to make a testcase, using the testcase from bug 460002, but I got all kinds of js errors, trying to get something. Probably might be able to come up with something, because he understands the testcase from bug 460002 that he wrote (I don't).
Comment 5 moz_bug_r_a4 2009-05-14 01:22:05 PDT
OK.  I'll attach an XSS testcase and a crash testcase.  The XSS testcase depends
on bfcache enabled.
Comment 10 Olli Pettay [:smaug] 2009-05-14 02:05:38 PDT
With the patch I can't reproduce crashes nor XSS.
Comment 11 Olli Pettay [:smaug] 2009-05-14 13:47:21 PDT
Created attachment 377510 [details] [diff] [review]
For 1.9.1 and 1.9.0
Comment 12 Johnny Stenback (:jst, 2009-05-18 22:32:27 PDT
Blocking 1.9.1 as well.
Comment 13 Johnny Stenback (:jst, 2009-05-18 23:27:07 PDT
Comment on attachment 376727 [details] [diff] [review]
for trunk

- In nsContentUtils::GetDocumentFromScriptContext():

+    nsCOMPtr<nsIDOMDocument> domdoc;
+    window->GetDocument(getter_AddRefs(domdoc));

I wonder if this should use nsPIDOMWindow::GetExtantDocument() to avoid ever creating a document if one doesn't exist yet (or any more) in some odd edgecases here?

Comment 14 Olli Pettay [:smaug] 2009-05-19 00:44:03 PDT
(In reply to comment #13)
> I wonder if this should use nsPIDOMWindow::GetExtantDocument() to avoid ever
> creating a document if one doesn't exist yet (or any more) in some odd
> edgecases here?
Yeah, perhaps. I just moved that code out from nsXMLHttpRequest.cpp
Comment 15 Olli Pettay [:smaug] 2009-05-20 05:20:35 PDT
I'll land the 1.9.1 patch to trunk too, and then after landing it to branches
I can back it out and land the trunk patch.
This way it gets more testing.
Comment 16 Olli Pettay [:smaug] 2009-05-20 09:00:43 PDT

Landed the 1.9.1 patch to trunk.
This bug is sort-of-fixed now, at least the security part of it,
but I will land the trunk patch later.
Comment 18 Daniel Veditz [:dveditz] 2009-05-22 11:03:25 PDT
Comment on attachment 377510 [details] [diff] [review]
For 1.9.1 and 1.9.0

Approved for, a=dveditz for release-drivers
Comment 19 Olli Pettay [:smaug] 2009-05-27 06:32:31 PDT
Checking in dom/src/offline/nsDOMOfflineResourceList.cpp;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.cpp,v  <--  nsDOMOfflineResourceList.cpp
new revision: 1.8; previous revision: 1.7
Comment 20 Olli Pettay [:smaug] 2009-06-14 14:01:55 PDT
Created attachment 383191 [details] [diff] [review]
for trunk (includes backout of the branch patch)
Comment 21 Olli Pettay [:smaug] 2009-06-15 01:48:38 PDT
Comment 22 Al Billings [:abillings] 2009-06-30 14:42:29 PDT
Are these test cases for 1.9.1 and trunk only? I get no alert or crash with with either case on Windows XP.
Comment 23 moz_bug_r_a4 2009-07-01 04:45:07 PDT
I can reproduce these testcases on  Did you allow Offline App?   On, when I loaded a testcase in a background tab, a notification for
offline-app-requested did not appear, thus I needed to reload the testcase to
allow Offline App.
Comment 24 Al Billings [:abillings] 2009-07-01 10:56:59 PDT
I uploaded them to my own web server on the net. The first time I run a case, I get prompted for offline access, which I allow. After that, with either case, when I click on the button, it simply opens a new tab with the case in that tab.

This is on Windows XP with 3.0.11. I wiped my XP virtual machine back to a clean state, reinstalled 3.0.11, and tried again today and had the same results.

I tried the same thing with a new profile on OS X 10.5.7. The behavior was the same. The first time a case is opened, I get the prompt for offline access. I choose "always allow" and then reload the page (I have exited and reloaded the page too). Clicking on the button in either case just loads the case again in a new tab. 

Is there a missing step somewhere for setup or the case?
Comment 25 moz_bug_r_a4 2009-07-01 12:39:38 PDT
When you uploaded the testcases to your server, did you modify *-opener.html? 
|var u = "?id=...";| is the URI of *-main.html in b.m.o, and you need to change
it to the URI of *-main.html in your server.
Comment 26 Al Billings [:abillings] 2009-07-01 12:52:52 PDT
Ah, yes. I didn't realize that you had hardcoded the URLs to BMO. When we have multi-file testcases, we normally run them off of BMO because we've had problems in the past with the interaction with BMO for some testcases.

When I load them from BMO, they work as you outline and the problems are fixed in the build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2009070105 GranParadiso/3.0.12pre (.NET CLR 3.5.30729).
Comment 27 Aakash Desai [:aakashd] 2009-07-21 10:33:09 PDT
verified FIXED using the attached testcases (and found the expected results matched the actual ones) on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721
Minefield/3.6a1pre ID:20090721044139


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20090720
Shiretoko/3.5.1pre ID:20090720042942

Note You need to log in before you can comment on or make changes to this bug.