Last Comment Bug 778023 - Need to figure out how cycle collected DOM files are supposed to play with workers
: Need to figure out how cycle collected DOM files are supposed to play with wo...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla17
Assigned To: Jan Varga [:janv]
:
:
Mentors:
Depends on:
Blocks: 726593 772434
  Show dependency treegraph
 
Reported: 2012-07-26 18:50 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2014-01-10 10:42 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
verified
+
verified
unaffected


Attachments
patch (6.36 KB, patch)
2012-07-30 11:33 PDT, Jan Varga [:janv]
bent.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 18:50:27 PDT
janv's file handle stuff created a cycle collected DOM file class, and baku has added another use of it.  This is great and all on the main thread, but trying to postMessage these to a worker thread causes Gecko to abort trying to AddRef a cycle collected object off the main thread.  We need to figure out a story here, ideally before janv's FileHandle ships in 15.
Comment 1 Jan Varga [:janv] 2012-07-26 21:40:19 PDT
DOM files created by file handle are valid only on the main thread.
GetInternalStream() must be called to create a new internal request.
It means that these objects have to be passed to other APIs immediately in the success handler. It doesn't make sense to pass these objects to workers.
We can just throw in that case.
Comment 2 Jan Varga [:janv] 2012-07-26 22:20:33 PDT
I mean that GetInternalStream() must be called *synchronously* in the success handler.
Otherwise, if it is called later asynchronously, the method returns an error.

I think there's other possibility to handle passing of these objects to workers.
We could create a new thread safe DOM file object during serialization, that would have correct type, name, size, but GetInternalStream() would just throw.
Comment 3 Jan Varga [:janv] 2012-07-28 10:49:40 PDT
I'm not sure if blobs returned by the ArchiveReader() (should) work in DOM workers.
If they do, then we could actually create a thread safe blob wrapper that would forward method impl to the real cycle collected blob. Destructor of this wrapper would proxy release the cycle collected blob when needed.

Does that sound ok ?
Comment 4 Alex Keybl [:akeybl] 2012-07-30 09:57:22 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0)
> We need to figure
> out a story here, ideally before janv's FileHandle ships in 15.

If left unfixed, what would the user impact be? Would backing out bug 726593 be a good alternative if this was left unfixed?
Comment 5 Jan Varga [:janv] 2012-07-30 09:59:42 PDT
I'm testing a fix, will attach a patch soon.

https://tbpl.mozilla.org/?tree=Try&rev=bbf5ec05ee07
Comment 6 Jan Varga [:janv] 2012-07-30 11:33:35 PDT
Created attachment 647226 [details] [diff] [review]
patch

fix per comment 1
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-30 16:06:22 PDT
(In reply to Jan Varga [:janv] from comment #3)
> I'm not sure if blobs returned by the ArchiveReader() (should) work in DOM
> workers.

They definitely should. A blob should always be a blob, there should be no differences between them.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 16:08:12 PDT
I don't think a backout is a good idea at this point.  Those patches were pretty big.
Comment 9 Alex Keybl [:akeybl] 2012-07-30 17:05:46 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> I don't think a backout is a good idea at this point.  Those patches were
> pretty big.

Thanks for the clarification. What would the user impact be here?
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 17:09:42 PDT
The impact is that using some of the features introduced with web workers will cause Firefox to abort (crash in a non-exploitable fashion).
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-01 17:35:30 PDT
Comment on attachment 647226 [details] [diff] [review]
patch

Review of attachment 647226 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +371,5 @@
>        // Get the raw nsISupports out of it.
>        nsISupports* wrappedObject = wrappedNative->Native();
>        NS_ASSERTION(wrappedObject, "Null pointer?!");
>  
> +      nsISupports* isupports;

Nit: "ccISupports"? And let's initialize it to nullptr so we don't have to check for QI failure.

@@ +374,5 @@
>  
> +      nsISupports* isupports;
> +      wrappedObject->QueryInterface(NS_GET_IID(nsCycleCollectionISupports),
> +                                    reinterpret_cast<void**>(&isupports));
> +      if (!isupports) {

How about we do a warning here too:

  if (ccISupports) {
    NS_WARNING(...)
  }
  else {
    ...
Comment 13 Ed Morley [:emorley] 2012-08-02 06:21:29 PDT
https://hg.mozilla.org/mozilla-central/rev/b83188166929
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-06 20:12:45 PDT
Please nominate for uplift so we can assess whether this will be able to land to branches, more specifically tomorrow's Beta.
Comment 15 Jan Varga [:janv] 2012-08-06 23:33:42 PDT
per discussion on IRC ...
The long term plan is to create a dummy threadsafe blob for blobs returned by FileHandle.getFile(), which would carry the name, type, size, etc.,
but it wouldn't be possible to read the blob in a worker, since the blob auto closed

However, on the other side, we should be able to pass ArchiveReader blobs to workers (and read content)
Comment 16 Jan Varga [:janv] 2012-08-07 00:37:41 PDT
Comment on attachment 647226 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 772434 and bug 726593

User impact if declined: 
Using some of the features introduced with web workers will cause Firefox to abort (crash in a non-exploitable fashion)

Testing completed (on m-c, etc.): 
Patch contains a mochitest and already landed on m-c

Risk to taking this patch (and alternatives if risky): 
There's very little risk. The patch logic is very simple and it just prevents cycle collected blobs (FileHandle and ArchiveReader ones) from passing to workers

String or UUID changes made by this patch: 
No string changes
Comment 17 Jan Varga [:janv] 2012-08-07 02:19:09 PDT
(In reply to Jan Varga [:janv] from comment #16)
> Risk to taking this patch (and alternatives if risky): 
> There's very little risk. The patch logic is very simple and it just
> prevents cycle collected blobs (FileHandle and ArchiveReader ones) from
> passing to workers

actually, only FileHandle ones, ArchiveReader is currently only on trunk
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-07 07:37:59 PDT
Comment on attachment 647226 [details] [diff] [review]
patch

low risk, has a test, approving.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-16 15:52:03 PDT
Mihaela, can you please make sure this is verified for Firefox 16 and 17 as well? Thanks.
Comment 23 Tracy Walker [:tracy] 2014-01-10 10:42:11 PST
mass remove verifyme requests greater than 4 months old

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