778023 - Need to figure out how cycle collected DOM files are supposed to play with workers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

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]]

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]]

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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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]]

I'm testing a fix, will attach a patch soon.

https://tbpl.mozilla.org/?tree=Try&rev=bbf5ec05ee07

Comment 6

[Jan Varga [:janv]]

Attachment: patch

fix per comment 1

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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 (Exited; not receiving bugmail, old account, do not use)]

I don't think a backout is a good idea at this point.  Those patches were pretty big.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

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!)]

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 12

[Jan Varga [:janv]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b83188166929

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b83188166929

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

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]]

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]]

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]]

(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]

Comment on attachment 647226 [details] [diff] [review]
patch

low risk, has a test, approving.

Comment 19

[Jan Varga [:janv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/940158b98658
https://hg.mozilla.org/releases/mozilla-beta/rev/edaab0f48feb

Comment 20

[Mihaela Velimiroviciu (:mihaelav)]

The mochitest attached to the fix passes on all systems:
* WinXP: https://tbpl.mozilla.org/php/getParsedLog.php?id=14335951&tree=Mozilla-Beta
* Win7: https://tbpl.mozilla.org/php/getParsedLog.php?id=14336125&tree=Mozilla-Beta
* Mac OSX 10.7.4: https://tbpl.mozilla.org/php/getParsedLog.php?id=14334098&tree=Mozilla-Beta
* Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=14334298&tree=Mozilla-Beta

Marking verified for Firefox 15.0(beta).

Comment 21

[u279076]

Mihaela, can you please make sure this is verified for Firefox 16 and 17 as well? Thanks.

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

No fails for the attached test on Mozilla-Release(16.0) and Mozilla-Beta(17.0), either.

Mozilla-Release branch:
* WinXP: https://tbpl.mozilla.org/php/getParsedLog.php?id=15999177&tree=Mozilla-Release
* Win7: https://tbpl.mozilla.org/php/getParsedLog.php?id=15998598&tree=Mozilla-Release
* Mac OSX 10.7: https://tbpl.mozilla.org/php/getParsedLog.php?id=15994856&tree=Mozilla-Release
* Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=15994814&tree=Mozilla-Release

Mozilla-Beta branch:
* Win XP: https://tbpl.mozilla.org/php/getParsedLog.php?id=16182170&tree=Mozilla-Beta
* Win7: https://tbpl.mozilla.org/php/getParsedLog.php?id=16181909&tree=Mozilla-Beta
* Mac OSX 10.7: https://tbpl.mozilla.org/php/getParsedLog.php?id=16176572&tree=Mozilla-Beta
* Linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=16188220&tree=Mozilla-Beta

Comment 23

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old