Closed Bug 942707 Opened 11 years ago Closed 10 years ago

[e10s] pdf viewer doesn't work with e10s enabled

Categories

(Firefox :: PDF Viewer, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
e10s + ---
firefox-esr31 --- disabled

People

(Reporter: markh, Assigned: jimm)

References

(Blocks 1 open bug)

Details

(Keywords: sec-audit, Whiteboard: [pdfjs-c-integration] [fullscreen support is bug 961362])

Attachments

(1 file, 32 obsolete files)

1.77 KB, patch
dao
: review+
Details | Diff | Splinter Review
If you attempt to view a PDF file that would normally open in the PDF viewer, it doesn't work.  A blank window is created and the PDF downloads.  The console includes:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Async version must be used"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///o:/src/mozilla-git/central/obj-release/dist/bin/components/nsHelperAppDlg.js :: nsUnknownContentTypeDialog.prototype.promptForSaveToFile :: line 194"  data: no]
************************************************************
JavaScript error: chrome://browser/content/browser.js, line 13643: sidebarBroadcaster is null

Running the tests in  browser/extensions/pdfjs/test/ also fail with similar messages when e10s is enabled
See also bug 944929
Priority: -- → P2
Whiteboard: [pdfjs-c-integration]
Are we hitting bug 596880 ?
(In reply to Yury Delendik (:yury) from comment #2)
> Are we hitting bug 596880 ?

I suspect so. Do you think you might be able to work on this, Yuri?
See Also: → 980161
I looked into this a little more. It seems a little harder than I realized. pdf.js has some integration with chrome that is troublesome for e10s. It does some special stuff with the findbar, telemetry, downloads, and private browsing. Most of this code is in PdfStreamConverter.jsm.

There's also the issue of how to load the pdf.js code in the content process. Right now, it resides in browser/, which means it doesn't automatically get loaded into content processes. We could manually load it by sending the URL of the chrome.manifest down to the child process, or by directly sending the URLs of the various JSMs. We could also try moving the code to toolkit/ so that it gets loaded automatically.

An additional complication is that the code lives in an external git repo.
Assignee: nobody → jmathies
Assignee: jmathies → nobody
Depends on: e10s-plugins
I talked to John some more about this. It sounds like the bug 558184 might be helpful here, but I think the two could be done independently. Bug 558184 doesn't address the problem that pdf.js needs to talk to chrome in order to manipulate the findbar and stuff. It also doesn't address the problem of how to load pdf.js code into the child process, since pdf.js is currently part of the browser/ omnijar, which isn't loaded by default into the child.
Assignee: nobody → jmathies
Blocks: 1028210
Attached patch wip (obsolete) — Splinter Review
Attached patch wip v.2 (obsolete) — Splinter Review
Getting there, findbar support added and various other issues addressed.

to do still:

- notification bar interaction?
- misc. get prefs calls still around, which work but the set pref calls go through the shim. I'm thinking of wrapping all prefs so there's no confusion.
- moving all the files down into toolkit still needs to happen.
- finding a better place to init the parent and child shims would be a nice plus.
- test local, I think this might have broken at some point.
Attachment #8444513 - Attachment is obsolete: true
Attachment #8445511 - Attachment is obsolete: true
Attached patch part 2 - build logic (obsolete) — Splinter Review
Attached patch part 3 - core (obsolete) — Splinter Review
There's still some work to do here but the basics are in place. 

Yury, curious what you think so far of what I'm doing here. Also curious how we should handle landing changes like this since pdfjs is kept in a 3rd party repo.
Flags: needinfo?(ydelendik)
Attached patch part 3 - core (obsolete) — Splinter Review
I think I have a good scheme in place here for making sure this works in both local and remote windows. Adding back support for local browsers actually helped simplify things a bit since the prefs work we do in Pdfjs.init() only needs to happen in the parent process. Assuming we always support local and remote, we can skip that when initializing in the child.
Attachment #8445999 - Attachment is obsolete: true
No longer depends on: e10s-plugins
Attached patch part 3 - core (obsolete) — Splinter Review
- added support for warning notifications when pdfs don't render properly.
Attachment #8446671 - Attachment is obsolete: true
Attached patch part 3 - core (obsolete) — Splinter Review
- fixed / cleared remaining areas of the code I had marked for testing.

next up existing tests, and potentially add some new ones.
Attachment #8446771 - Attachment is obsolete: true
Comment on attachment 8446783 [details] [diff] [review]
part 3 - core

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

Bill Walker asked me to take a look at this given that Brendan and Yury are away at the moment.

Although I worked on PDF.JS I never looked at the Firefox integration code, which gets changed here. Therefore, I don't think I am the person that can give a r+ on the final patch, but I still give my feedback here in the hope it is useful.

Overall:
- the quality of the patch looks good!
- there are some trailing whitespaces, that could be removed for the final patch
-

::: toolkit/jsplugins/pdfjs/content/PdfJs.jsm
@@ +158,5 @@
> +    }
> +    if (this.enabled) {
> +      this._ensureRegistered();
> +    } else {
> +      this._ensureUnregistered();

How can |this._ensureUnregistered();| be called here? 

Also, the function is called |registerFactories|, so I don't see why a function called |_ensureUnregistered()| gets called here (seems like doing the exact opposite, no?)

::: toolkit/jsplugins/pdfjs/content/PdfjsChildUtils.jsm
@@ +88,5 @@
> +                       .sameTypeRootTreeItem
> +                       .QueryInterface(Ci.nsIDocShell)
> +                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIContentFrameMessageManager);
> +    winmm.addMessageListener("PDFJS:Child:warnResponse", function onEvent(aMsg) {

So, you add a new message listener each time |displayWarning()| is called and remove it once you get back a message from the parent. I wonder if it is possible to end up in case where |displayWarning()| is called twice, the first registered |onEvent(aMsg)| function is not unregistered yet and when |PDFJS:Child:warnResponse| is dispatched it causes both |onEvent(aMsg)| to be invoked.


Looking at |PdfjsParentUtils._displayWarning| I think the code as it is should be save.

Nevertheless, is it possible to query for |winm| once in the |PdfjsChildUtils.init()| function and attach the |winmm.addMessageListener("PDFJS:Child:warnResponse"| listener there once?


NOTE: I am not sure if it's possible to refactor/query for |winm| in the |init()| function and add the listener only once - my e10s knowledge goes to zero but I hope you know more about these details ;)

@@ +114,5 @@
> +      let suitcase = {
> +        _window: null,
> +        setChromeWindow: function (aObj) { this._window = aObj; }
> +      }
> +      if (!this._mm.sendSyncMessage("PDFJS:Parent:getChromeWindow", {}, { suitcase: suitcase })[0]) {

nit: way more than 80 chars. Might break the line?

@@ +130,5 @@
> +                         .getInterface(Ci.nsIDocShell)
> +                         .sameTypeRootTreeItem
> +                         .QueryInterface(Ci.nsIDocShell)
> +                         .QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIContentFrameMessageManager);

In case it's possible to query for |winm| once in the |init()| method, these lines here can go away as well :)

::: toolkit/jsplugins/pdfjs/content/PdfjsParentUtils.jsm
@@ +136,5 @@
> +  },
> +
> +  _getFindBar: function (aMsg) {
> +    // We send this over via the window's message manager, so target should
> +    // should be the dom window.

nit: remove the duplicate |should| on the second line please.
Comment on attachment 8446783 [details] [diff] [review]
part 3 - core

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

One more overall comment. You are using the terms |parent| and |child|. How do you feel about using |Chrome| and |Content| instead, such that you end up with

  |PdfjsParentUtils| -> |PdfjsChromeUtils|
  |PdfjsChildUtils|  -> |PdfjsContentUtils|
(In reply to julian.viereck from comment #20)
> Comment on attachment 8446783 [details] [diff] [review]
> part 3 - core
> 
> Review of attachment 8446783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One more overall comment. You are using the terms |parent| and |child|. How
> do you feel about using |Chrome| and |Content| instead, such that you end up
> with
> 
>   |PdfjsParentUtils| -> |PdfjsChromeUtils|
>   |PdfjsChildUtils|  -> |PdfjsContentUtils|

Love it, I wasn't a fan of the previous naming either. 

I'll get to the rest of your comments shortly, thanks for the feedback! I have tests passing now although I have a dom window leak to track down when running in-process. Once I get that sorted out I'll post a new part 3 wip and address comments.
Attached patch part 2 - build logic (obsolete) — Splinter Review
Attachment #8445998 - Attachment is obsolete: true
Attachment #8446783 - Attachment is obsolete: true
Attached patch part 3 - core changes (obsolete) — Splinter Review
Attached patch part 4 - update tests (obsolete) — Splinter Review
Comment on attachment 8448153 [details] [diff] [review]
part 3 - core changes

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

::: toolkit/jsplugins/pdfjs/content/PdfJs.jsm
@@ +232,5 @@
>    },
>  
>    // nsIObserver
>    observe: function observe(aSubject, aTopic, aData) {
> +    if (this.enabled) {

I think the lines of the |if (this.enabled) {| are the same as |registerFactories()|? If so, can you maybe call the |registerFactories()| here then to reuse code?


Also carrying the question from last review over: Can you think about a better name than |registerFactories()|? The function calls |this._ensureUnregistered();|, which is a contradiction to the function name |registerFactories()| :/


Not sure, but how do you feel about:

  registerFactories() -> updateRegistration()

or

  registerFactories() -> updateFactoriesRegistration()

?
(In reply to Julian Viereck from comment #19)
> Comment on attachment 8446783 [details] [diff] [review]
> part 3 - core
> 
> Review of attachment 8446783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/jsplugins/pdfjs/content/PdfJs.jsm
> @@ +158,5 @@
> > +    }
> > +    if (this.enabled) {
> > +      this._ensureRegistered();
> > +    } else {
> > +      this._ensureUnregistered();
> 
> How can |this._ensureUnregistered();| be called here? 

If pdfjs is disabled via prefs, _ensureUnregistered will get called. We call registerFactories from three places, in the parent process when pdfjs-loader.js is loaded into local frame script, via browser-child.js in the content process, and when settings observers in the parent pick up settings have changed, we forward an event over to content telling it refresh its registration.

> Also, the function is called |registerFactories|, so I don't see why a
> function called |_ensureUnregistered()| gets called here (seems like doing
> the exact opposite, no?)

I'll rename this to something more generic: how about 'updateRegistration()' which would then call the internal methods _ensure(Un/R)egistered?

> ::: toolkit/jsplugins/pdfjs/content/PdfjsChildUtils.jsm
> @@ +88,5 @@
> > +                       .sameTypeRootTreeItem
> > +                       .QueryInterface(Ci.nsIDocShell)
> > +                       .QueryInterface(Ci.nsIInterfaceRequestor)
> > +                       .getInterface(Ci.nsIContentFrameMessageManager);
> > +    winmm.addMessageListener("PDFJS:Child:warnResponse", function onEvent(aMsg) {
> 
> So, you add a new message listener each time |displayWarning()| is called
> and remove it once you get back a message from the parent. I wonder if it is
> possible to end up in case where |displayWarning()| is called twice, the
> first registered |onEvent(aMsg)| function is not unregistered yet and when
> |PDFJS:Child:warnResponse| is dispatched it causes both |onEvent(aMsg)| to
> be invoked.

Yes I think you're right, let me see what I can come up with.

> Nevertheless, is it possible to query for |winm| once in the
> |PdfjsChildUtils.init()| function and attach the
> |winmm.addMessageListener("PDFJS:Child:warnResponse"| listener there once?

PdfjsChildUtils is a global in the content process and as such is associated with multiple windows. I think passing the target dom window in here is the right approach. I just need to account for the possibility that multiple tabs/windows might have notifications displayed at the same time.
(In reply to Jim Mathies [:jimm] from comment #26)
> (In reply to Julian Viereck from comment #19)
> > ::: toolkit/jsplugins/pdfjs/content/PdfjsChildUtils.jsm
> > @@ +88,5 @@
> > > +                       .sameTypeRootTreeItem
> > > +                       .QueryInterface(Ci.nsIDocShell)
> > > +                       .QueryInterface(Ci.nsIInterfaceRequestor)
> > > +                       .getInterface(Ci.nsIContentFrameMessageManager);
> > > +    winmm.addMessageListener("PDFJS:Child:warnResponse", function onEvent(aMsg) {
> > 
> > So, you add a new message listener each time |displayWarning()| is called
> > and remove it once you get back a message from the parent. I wonder if it is
> > possible to end up in case where |displayWarning()| is called twice, the
> > first registered |onEvent(aMsg)| function is not unregistered yet and when
> > |PDFJS:Child:warnResponse| is dispatched it causes both |onEvent(aMsg)| to
> > be invoked.
> 
> Yes I think you're right, let me see what I can come up with.

Wrapping the callback in a cpow "just worked", which is pretty cool. :)
Attached patch part 3 - toolkit changes (obsolete) — Splinter Review
Splitting up the toolkit changes and pdfjs code changes for reviews.
Attachment #8448153 - Attachment is obsolete: true
Attachment #8448154 - Attachment is obsolete: true
Attached patch part 4 - pdfjs changes (obsolete) — Splinter Review
Attached patch part 5 - pdfjs test changes (obsolete) — Splinter Review
Attachment #8448862 - Attachment description: part 4 - pdfjs change → part 4 - pdfjs changes
Comment on attachment 8448863 [details] [diff] [review]
part 5 - pdfjs test changes

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

Does the current tests include an e10s integration test already? If not, do you think it is worth adding one?
Comment on attachment 8448862 [details] [diff] [review]
part 4 - pdfjs changes

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

Overall, looks good.

::: toolkit/jsplugins/pdfjs/content/PdfjsChromeUtils.jsm
@@ +76,5 @@
> +      this._ppmm.removeMessageListener("PDFJS:Parent:isDefaultHandlerApp", this);
> +
> +      this._mmg.removeMessageListener("PDFJS:Parent:getChromeWindow", this);
> +      this._mmg.removeMessageListener("PDFJS:Parent:getFindBar", this);
> +      this._mmg.removeMessageListener("PDFJS:Parent:displayWarning", this);

In the |init| function, you are adding an observer for "quit-application". Is there need to remove this listener in the |uninit()| method as well?

@@ +159,5 @@
> +  },
> +
> +  _getFindBar: function (aMsg) {
> +    // We send this over via the window's message manager, so target should
> +    // should be the dom window.

Still twice "should" here.

@@ +210,5 @@
> +   */
> +  _displayWarning: function (aMsg) {
> +    let json = aMsg.json;
> +    let browser = aMsg.target;
> +    let cpowCB = aMsg.objects.callback;

|cpowCB| is correct but sitll pretty cryptic. How do you feel about about |contentCallback|?
Comment on attachment 8448861 [details] [diff] [review]
part 3 - toolkit changes

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

::: toolkit/content/pdfjs-loader.js
@@ +15,5 @@
> +
> +// Both of the following calls have checks to insure registration
> +// only happens once.
> +
> +// parent only: configure default prefs and set up pref observers

Please be consistent with the comments: Start with a capital letter and end the sentence with a period. Same for the comment below.
(In reply to Julian Viereck from comment #34)
> Comment on attachment 8448861 [details] [diff] [review]
> part 3 - toolkit changes
> 
> Review of attachment 8448861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/pdfjs-loader.js
> @@ +15,5 @@
> > +
> > +// Both of the following calls have checks to insure registration
> > +// only happens once.
> > +
> > +// parent only: configure default prefs and set up pref observers
> 
> Please be consistent with the comments: Start with a capital letter and end
> the sentence with a period. Same for the comment below.

We don't have a harness to run tests in the content process, but we we do test functionality of remote content using the existing tests when e10s mode is enabled. So for example if registration in the child was broken, pdfjs wouldn't load and various tests would fail, which gives us ok coverage.
(In reply to Julian Viereck from comment #33)
> Comment on attachment 8448862 [details] [diff] [review]
> part 4 - pdfjs changes
> 
> Review of attachment 8448862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, looks good.
> 
> ::: toolkit/jsplugins/pdfjs/content/PdfjsChromeUtils.jsm
> @@ +76,5 @@
> > +      this._ppmm.removeMessageListener("PDFJS:Parent:isDefaultHandlerApp", this);
> > +
> > +      this._mmg.removeMessageListener("PDFJS:Parent:getChromeWindow", this);
> > +      this._mmg.removeMessageListener("PDFJS:Parent:getFindBar", this);
> > +      this._mmg.removeMessageListener("PDFJS:Parent:displayWarning", this);
> 
> In the |init| function, you are adding an observer for "quit-application".
> Is there need to remove this listener in the |uninit()| method as well?

nice catch, added.

> @@ +159,5 @@
> > +  },
> > +
> > +  _getFindBar: function (aMsg) {
> > +    // We send this over via the window's message manager, so target should
> > +    // should be the dom window.
> 
> Still twice "should" here.

fixed.

> @@ +210,5 @@
> > +   */
> > +  _displayWarning: function (aMsg) {
> > +    let json = aMsg.json;
> > +    let browser = aMsg.target;
> > +    let cpowCB = aMsg.objects.callback;
> 
> |cpowCB| is correct but sitll pretty cryptic. How do you feel about about
> |contentCallback|?

cpowCallback? I like annotating variables that cpows so people know what they are working with.
(In reply to Jim Mathies [:jimm] from comment #36)
> > @@ +210,5 @@
> > > +   */
> > > +  _displayWarning: function (aMsg) {
> > > +    let json = aMsg.json;
> > > +    let browser = aMsg.target;
> > > +    let cpowCB = aMsg.objects.callback;
> > 
> > |cpowCB| is correct but sitll pretty cryptic. How do you feel about about
> > |contentCallback|?
> 
> cpowCallback? I like annotating variables that cpows so people know what
> they are working with.

+1 for |cpowCallback| :)
(In reply to Jim Mathies [:jimm] from comment #35)
> (In reply to Julian Viereck from comment #34)
> > Comment on attachment 8448861 [details] [diff] [review]
> > part 3 - toolkit changes
> > 
> > Review of attachment 8448861 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/content/pdfjs-loader.js
> > @@ +15,5 @@
> > > +
> > > +// Both of the following calls have checks to insure registration
> > > +// only happens once.
> > > +
> > > +// parent only: configure default prefs and set up pref observers
> > 
> > Please be consistent with the comments: Start with a capital letter and end
> > the sentence with a period. Same for the comment below.
> 
> We don't have a harness to run tests in the content process, but we we do
> test functionality of remote content using the existing tests when e10s mode
> is enabled. So for example if registration in the child was broken, pdfjs
> wouldn't load and various tests would fail, which gives us ok coverage.

Cool, thanks for the explanation. Think that should be enough then.
Whiteboard: [pdfjs-c-integration] → [pdfjs-c-integration] [fullscreen support is bug 961362]
Comment on attachment 8445996 [details] [diff] [review]
part 1 - move pdfjs resources to toolkit

This is just file moves, but I'd like to get sign off on the root toolkit dir I've added named 'jsplugins'.
Attachment #8445996 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8448143 [details] [diff] [review]
part 2 - build logic

Request review on build config changes for moving pdfjs from browser to toolkit/jsplugins/pdfjs. Note browser is the only project that uses this code currently.
Attachment #8448143 - Flags: review?(gps)
Attached patch part 3 - toolkit changes v.1 (obsolete) — Splinter Review
Changes to browser and remote-browser for loading pdfjs from toolkit.

Dao, are you the right reviewer here? Feel free to reassign if not.
Attachment #8448861 - Attachment is obsolete: true
Attachment #8448862 - Attachment is obsolete: true
Attachment #8448863 - Attachment is obsolete: true
Attachment #8448917 - Flags: review?(dao)
Attached patch part 4 - pdfjs changes v.3 (obsolete) — Splinter Review
Thanks for all the help so far Julian. How should we handle final review here, do you feel comfortable signing off on the pdfjs specific changes or would you prefer I wait for yury to return from pto?
Attachment #8448920 - Flags: feedback?(julian.viereck)
Comment on attachment 8448920 [details] [diff] [review]
part 4 - pdfjs changes v.3

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

Talked to this with Jim on IRC: This patch will get applied to the code in the GitHub mozilla/pdf.js repo. From the code in the repo, a "nightly" version of PDF.JS is build. To not break PDF.JS for the users of this "nightly" PDF.JS extension, we must make sure the extension is still functional after applying the PDF.JS changes in this patch without running the other patches to Firefox.

As far as I can read the code, I don't think this is the case at the moment.
Attachment #8448920 - Flags: feedback?(julian.viereck)
Comment on attachment 8448143 [details] [diff] [review]
part 2 - build logic

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

Redirecting build review since I'm disposed with other goals right now.
Attachment #8448143 - Flags: review?(gps) → review?(mh+mozilla)
Comment on attachment 8448143 [details] [diff] [review]
part 2 - build logic

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

We usually don't put MOZ_BUILD_APP-specific stuff in toolkit, and I'm not really sure why moving this code is required at all for e10s. Punting to bsmedberg.
Attachment #8448143 - Flags: review?(mh+mozilla) → review?(benjamin)
(In reply to Julian Viereck from comment #43)
> Comment on attachment 8448920 [details] [diff] [review]
> part 4 - pdfjs changes v.3
> 
> Review of attachment 8448920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Talked to this with Jim on IRC: This patch will get applied to the code in
> the GitHub mozilla/pdf.js repo. From the code in the repo, a "nightly"
> version of PDF.JS is build. To not break PDF.JS for the users of this
> "nightly" PDF.JS extension, we must make sure the extension is still
> functional after applying the PDF.JS changes in this patch without running
> the other patches to Firefox.
> 
> As far as I can read the code, I don't think this is the case at the moment.

Addressing this looks pretty easy, we need to add a couple new empty jsms for the content and chrome helpers. The existing PdfJs.jsm in the extension should be working. This isn't going to get the extension running remote. I'd prefer to look at that in a follow up though so I can get mc work landed.


(In reply to Mike Hommey [:glandium] from comment #45)
> Comment on attachment 8448143 [details] [diff] [review]
> part 2 - build logic
> 
> Review of attachment 8448143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We usually don't put MOZ_BUILD_APP-specific stuff in toolkit, and I'm not
> really sure why moving this code is required at all for e10s. Punting to
> bsmedberg.

We discussed this in our e10s meeting, the consensus was moving it to toolkit would be good so that other apps could use it if they want.

However if that was premature, it would be pretty easy to undo that change. The initialization work can happen in tabbrowser vs. browser/remote-browser.
Comment on attachment 8445996 [details] [diff] [review]
part 1 - move pdfjs resources to toolkit

I think I'd prefer "contentviewers" to "jsplugins"
Attachment #8445996 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 8448917 [details] [diff] [review]
part 3 - toolkit changes v.1

Please don't use defineLazyModuleGetter when you're accessing the object synchronously in the next step.

Is resource://pdf.js/PdfJs.jsm guaranteed to be available in any application building and potentially using toolkit/content/browser-child.js? I'm not really familiar with how we integrate pdf.js.
Attachment #8448917 - Flags: review?(dao) → review-
(In reply to Mike Hommey [:glandium] from comment #45)
> Comment on attachment 8448143 [details] [diff] [review]
> part 2 - build logic
> 
> Review of attachment 8448143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We usually don't put MOZ_BUILD_APP-specific stuff in toolkit, and I'm not
> really sure why moving this code is required at all for e10s. Punting to
> bsmedberg.

Good point, plus I realized I can't easily disable tab script using the preprocessor going about it this way. I think what I'd like to do is create a config option but I'll wait to update until we make a final decision on where we want pdf.js.

blassey mentioned the other day that the fennec team was interested in integrating pdf.js, in which case I think a move to toolkit makes sense.
(In reply to Dão Gottwald [:dao] from comment #49)
> Comment on attachment 8448917 [details] [diff] [review]
> part 3 - toolkit changes v.1
> 
> Please don't use defineLazyModuleGetter when you're accessing the object
> synchronously in the next step.

Thanks, will update.

> Is resource://pdf.js/PdfJs.jsm guaranteed to be available in any application
> building and potentially using toolkit/content/browser-child.js? I'm not
> really familiar with how we integrate pdf.js.

No it won't, see comment 50 for how I plan to address this.
Comment on attachment 8448143 [details] [diff] [review]
part 2 - build logic

We should not have MOZ_BUILD_APP ifdefs in toolkit. Really, toolkit/ should be the same for all apps.

I really think we'd be better off just putting this in toplevel/pdfjs and then building it from each app that wants to include it.

Putting pdfjs into the toolkit just to solve an e10s problem doesn't sound quite right. There should be a way to have an app-specific js plugin. So maybe we don't need to move it at all, but just have a way to register pdfjs to work in content processes.
Attachment #8448143 - Flags: review?(benjamin) → review-
Status: NEW → ASSIGNED
Attached patch wip (obsolete) — Splinter Review
- moved back to browser
Attachment #8445996 - Attachment is obsolete: true
Attachment #8448143 - Attachment is obsolete: true
Attachment #8448917 - Attachment is obsolete: true
Attachment #8448920 - Attachment is obsolete: true
Attached patch wip v.2 (obsolete) — Splinter Review
- added restrictions to pref access via the child apis we expose
- cleaned up tests post the move back to browser
- general cleanup

https://tbpl.mozilla.org/?tree=Try&rev=1282f1fac4a4
Attachment #8456325 - Attachment is obsolete: true
Attached patch part 1 - browser changes v.1 (obsolete) — Splinter Review
We require access to message manager for getting things loaded in the child, so this patch moves PdfJs initialization to tabbrowser.
Attachment #8456389 - Attachment is obsolete: true
Attachment #8457466 - Flags: review?(mconley)
Attached patch part 2 - pdfjs changes v.1 (obsolete) — Splinter Review
Not a lot has changed here since the last rev. Loading behavior has changed somewhat, but everything else remains the same.
Attachment #8457467 - Flags: review?(julian.viereck)
Attached patch part 3 - test updates v.1 (obsolete) — Splinter Review
Updates to test to get things working with --e10s. I also took the chance to clean these tests up a little.
Attachment #8457470 - Flags: review?(mconley)
Comment on attachment 8457466 [details] [diff] [review]
part 1 - browser changes v.1

I think this belongs in browser.js (gBrowserInit.onLoad). tabbrowser.xml doesn't need to know about this.
Attachment #8457466 - Flags: review?(mconley) → review-
Attached patch part 1 - browser changes v.2 (obsolete) — Splinter Review
- updated per dao's comment, moved the tab browser changes to browser.js.
Attachment #8457466 - Attachment is obsolete: true
Attachment #8457494 - Flags: review?(mconley)
Comment on attachment 8457494 [details] [diff] [review]
part 1 - browser changes v.2

Please move your code up at least before this._boundDelayedStartup = ..., as that's supposed to be the last step of this method.
I think next to mm.loadFrameScript("chrome://browser/content/content.js", true); might be a good place.

>+    // Initialize PdfJs when running in-process and remote. If the PdfJs
>+    // extension is installed init below will get overridden leaving
>+    // initialization to the extension.
>+    let jsm = "resource://pdf.js/PdfJs.jsm";
>+    let PdfJs = Components.utils.import(jsm, {}).PdfJs;

This fits on one line:

    let PdfJs = Cu.import("resource://pdf.js/PdfJs.jsm", {}).PdfJs;

>+      let wmm = window.messageManager;
>+      wmm.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js", true);

ditto:

      messageManager.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js", true);
Attachment #8457494 - Flags: review?(mconley) → review-
(In reply to Dão Gottwald [:dao] from comment #60)
> Comment on attachment 8457494 [details] [diff] [review]
> part 1 - browser changes v.2
> 
> Please move your code up at least before this._boundDelayedStartup = ..., as
> that's supposed to be the last step of this method.
> I think next to mm.loadFrameScript("chrome://browser/content/content.js",
> true); might be a good place.

ok, moved it just below the content.js frame load call.

> This fits on one line:
> 
>     let PdfJs = Cu.import("resource://pdf.js/PdfJs.jsm", {}).PdfJs;

Moved up, this comes in at 82 characters in length, so I wrapped it at Components.

> >+      let wmm = window.messageManager;
> >+      wmm.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js", true);
> ditto:
> 
>      
> messageManager.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js",
> true);

This line comes in at 88 characters, is that ok in browser.js? In metro's code we required lines of js to max out 80 max except in special circumstances.
(In reply to Jim Mathies [:jimm] from comment #61)
> > This fits on one line:
> > 
> >     let PdfJs = Cu.import("resource://pdf.js/PdfJs.jsm", {}).PdfJs;
> 
> Moved up, this comes in at 82 characters in length, so I wrapped it at
> Components.

you should use Cu

> > >+      let wmm = window.messageManager;
> > >+      wmm.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js", true);
> > ditto:
> > 
> >      
> > messageManager.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js",
> > true);
> 
> This line comes in at 88 characters, is that ok in browser.js? In metro's
> code we required lines of js to max out 80 max except in special
> circumstances.

Yes, 80 is still the guideline, I think, but we're a bit more lax and often don't treat it as a hard limit.
(In reply to Dão Gottwald [:dao] from comment #62)
> (In reply to Jim Mathies [:jimm] from comment #61)
> > > This fits on one line:
> > > 
> > >     let PdfJs = Cu.import("resource://pdf.js/PdfJs.jsm", {}).PdfJs;
> > 
> > Moved up, this comes in at 82 characters in length, so I wrapped it at
> > Components.
> 
> you should use Cu

ah ok, sorry missed that in your original comment. 

> > > >+      let wmm = window.messageManager;
> > > >+      wmm.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js", true);
> > > ditto:
> > >      
> > > messageManager.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js",
> > > true);
> > 
> > This line comes in at 88 characters, is that ok in browser.js? In metro's
> > code we required lines of js to max out 80 max except in special
> > circumstances.
> 
> Yes, 80 is still the guideline, I think, but we're a bit more lax and often
> don't treat it as a hard limit.

wfm, updated.
Attached patch browser changes v.3 (obsolete) — Splinter Review
3rd time's the charm?
Attachment #8457494 - Attachment is obsolete: true
Attachment #8457580 - Flags: review?(mconley)
See Also: → 952844
Comment on attachment 8457580 [details] [diff] [review]
browser changes v.3

>--- a/browser/base/content/browser.js	Wed Jul 16 14:00:26 2014 -0500
>+++ b/browser/base/content/browser.js	Wed Jul 16 16:37:24 2014 -0500
>@@ -784,6 +784,22 @@
>     let mm = window.getGroupMessageManager("browsers");
>     mm.loadFrameScript("chrome://browser/content/content.js", true);
> 
>+    // Initialize PdfJs when running in-process and remote. If the PdfJs
>+    // extension is installed init below will get overridden leaving
>+    // initialization to the extension.
>+    let PdfJs = Cu.import("resource://pdf.js/PdfJs.jsm", {}).PdfJs;
>+    // parent only: configure default prefs, set up pref observers,
>+    // register pdf content handlers, and initializes parent side
>+    // message manager shims for privileged api access.
>+    PdfJs.init(gMultiProcessBrowser);
>+    if (gMultiProcessBrowser) {
>+      // If we're running remote, PdfJs also needs to load and register
>+      // in the child. Similar to the call above for the parent, inits
>+      // settings, registers content handlers, and inits message
>+      // manager child shim for privileged api access.
>+      window.messageManager.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js",true);

Please remove "window." and add a space after the comma.

And are you sure that we shouldn't use mm rather than messageManager?
Attachment #8457580 - Flags: review?(mconley) → review+
(In reply to Dão Gottwald [:dao] from comment #65)
> 
> And are you sure that we shouldn't use mm rather than messageManager?

No I don't think so. The browsers group points to each tab frame, where as I'm just trying to register a global content handler and initialize a module that uses the child process message manager. Technically I don't think this needs to happen for every window, it only needs to happen once. Unfortunately I don't know of a way to get at the parent side of the child process manager from js.
(In reply to Jim Mathies [:jimm] from comment #66)
> Technically I don't think this
> needs to happen for every window, it only needs to happen once.
> Unfortunately I don't know of a way to get at the parent side of the child
> process manager from js.

It sounds like you should put this in a browser-window-before-show observer in nsBrowserGlue.js, similarly to the browser-delayed-startup-finished observer there.
(In reply to Jim Mathies [:jimm] from comment #64)
> Created attachment 8457580 [details] [diff] [review]
> browser changes v.3
> 
> 3rd time's the charm?

In the context of comment #43: It looks like this version of the browser changes work well with current PDF.JS extensions build from the GitHub repository, right? So no crashing extensions - that's good :)
Flags: needinfo?(jmathies)
Comment on attachment 8457467 [details] [diff] [review]
part 2 - pdfjs changes v.1

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

Please correct me if I am wrong here: The plan is to land the changes to the PDF.JS extensions in Mozilla Central and then apply the changes also to the PDF.JS GitHub repo, right?

The extensions from the GitHub repo works with older versions of Firefox as well. If we land these changes to the GitHub repo and if I read this patch correctly, then a version of Firefox is required, which defines things like

  Services.appinfo.processType 
  Cc["@mozilla.org/parentprocessmessagemanager;1"]

do you have an idea which version of Firefox is required for these kind of e10s functionality?


Overall, I don't feel confident enough to do an review on this patch. The code has to work when integrated in the browser and when loaded as an extension from the GitHub repo. The first case I am confident it will work, but I cannot figure out the later case. Therefore, I think it is best to wait until Yury comes back (which should be next week). Moving review over to Yury.
Attachment #8457467 - Flags: review?(julian.viereck) → review?(ydelendik)
(In reply to Dão Gottwald [:dao] from comment #67)
> (In reply to Jim Mathies [:jimm] from comment #66)
> > Technically I don't think this
> > needs to happen for every window, it only needs to happen once.
> > Unfortunately I don't know of a way to get at the parent side of the child
> > process manager from js.
> 
> It sounds like you should put this in a browser-window-before-show observer
> in nsBrowserGlue.js, similarly to the browser-delayed-startup-finished
> observer there.

Yes that looks perfect!
Flags: needinfo?(jmathies)
(In reply to Julian Viereck from comment #68)
> (In reply to Jim Mathies [:jimm] from comment #64)
> > Created attachment 8457580 [details] [diff] [review]
> > browser changes v.3
> > 
> > 3rd time's the charm?
> 
> In the context of comment #43: It looks like this version of the browser
> changes work well with current PDF.JS extensions build from the GitHub
> repository, right? So no crashing extensions - that's good :)

Yes, I did testing with the current extension, in-process use worked fine.
(In reply to Julian Viereck from comment #69)
> Comment on attachment 8457467 [details] [diff] [review]
> part 2 - pdfjs changes v.1
> 
> Review of attachment 8457467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please correct me if I am wrong here: The plan is to land the changes to the
> PDF.JS extensions in Mozilla Central and then apply the changes also to the
> PDF.JS GitHub repo, right?

That sounds good to me. I don't have control over the github repo so whoever does is free to decide how they want to integrate (or not integrate) these changes. It would be great if these changes were there so merges from github to mc continue to be simple to do.

 
> The extensions from the GitHub repo works with older versions of Firefox as
> well. If we land these changes to the GitHub repo and if I read this patch
> correctly, then a version of Firefox is required, which defines things like
> 
>   Services.appinfo.processType 
>   Cc["@mozilla.org/parentprocessmessagemanager;1"]
> 
> do you have an idea which version of Firefox is required for these kind of
> e10s functionality?

3.7 would be the first rev with mm in it, although we've fixed a lot of bugs along the way. It probably wouldn't run in that version.

> Overall, I don't feel confident enough to do an review on this patch. The
> code has to work when integrated in the browser and when loaded as an
> extension from the GitHub repo. The first case I am confident it will work,
> but I cannot figure out the later case. Therefore, I think it is best to
> wait until Yury comes back (which should be next week). Moving review over
> to Yury.

Sounds good, thanks for all the help thus far!
(In reply to Jim Mathies [:jimm] from comment #70)
> (In reply to Dão Gottwald [:dao] from comment #67)
> > (In reply to Jim Mathies [:jimm] from comment #66)
> > > Technically I don't think this
> > > needs to happen for every window, it only needs to happen once.
> > > Unfortunately I don't know of a way to get at the parent side of the child
> > > process manager from js.
> > 
> > It sounds like you should put this in a browser-window-before-show observer
> > in nsBrowserGlue.js, similarly to the browser-delayed-startup-finished
> > observer there.
> 
> Yes that looks perfect!

Hmm, if session restore tried to load a pdf, would this get called early enough to initialize the pdfjs modules?
Flags: needinfo?(ydelendik) → needinfo?(dao)
(In reply to Jim Mathies [:jimm] from comment #73)
> (In reply to Jim Mathies [:jimm] from comment #70)
> > (In reply to Dão Gottwald [:dao] from comment #67)
> > > (In reply to Jim Mathies [:jimm] from comment #66)
> > > > Technically I don't think this
> > > > needs to happen for every window, it only needs to happen once.
> > > > Unfortunately I don't know of a way to get at the parent side of the child
> > > > process manager from js.
> > > 
> > > It sounds like you should put this in a browser-window-before-show observer
> > > in nsBrowserGlue.js, similarly to the browser-delayed-startup-finished
> > > observer there.
> > 
> > Yes that looks perfect!
> 
> Hmm, if session restore tried to load a pdf, would this get called early
> enough to initialize the pdfjs modules?

Yes, session restore only uses this notification to start reading the stored session from disk. For actually restoring stuff it waits for browser-delayed-startup-finished.

I think you might also want to just hook into nsBrowserGlue's browser-delayed-startup-finished observer, because initializing PdfJS shouldn't block making the first window visible (before any web content gets loaded).
Flags: needinfo?(dao)
Attached patch part 1: browser changes v.4 (obsolete) — Splinter Review
- moved init code to nsBrowserGlue _onFirstWindowLoaded.

this worked with multiple windows open and passes all tests locally.
Attachment #8457580 - Attachment is obsolete: true
Attachment #8458009 - Flags: review?(mconley)
Comment on attachment 8457470 [details] [diff] [review]
part 3 - test updates v.1

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

Hm - browser_pdfjs_views.js and browser_pdfjs_main.js are failing for me when I run it without --e10s - it's complaining about a window leak:

0:09.98 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_views.js | leaked until shutdown [nsGlobalWindow #17 outer  http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf]
 0:09.98 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_views.js | leaked until shutdown [nsGlobalWindow #19 inner http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf]

So I think we're holding onto something that we shouldn't... an interesting clue is that browser_pdfjs_savedialog.js exits just fine without --e10s.

Otherwise, the code looks fine - though I have some small suggestions.

Thanks Jim!

::: browser/extensions/pdfjs/test/browser_pdfjs_main.js
@@ +6,4 @@
>  const RELATIVE_DIR = "browser/extensions/pdfjs/test/";
>  const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
>  
> +let gTab = null;

Instead of adding another global, how about we pass the tab to runTests instead? We lose a global that way, and you don't have to clean it out in the cleanup function.

::: browser/extensions/pdfjs/test/browser_pdfjs_views.js
@@ +6,4 @@
>  const RELATIVE_DIR = "browser/extensions/pdfjs/test/";
>  const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
>  
> +let gTab = null;

Same suggestion applies to get rid of the global and pass the tab to runTests instead.

@@ +26,4 @@
>    });
>  
> +  gTab = gBrowser.addTab(TESTROOT + "file_pdfjs_test.pdf");
> +  var newTabBrowser = gBrowser.getBrowserForTab(gTab);

While you're changing the rest of the vars, might as well turn this into a let too.
Attachment #8457470 - Flags: review?(mconley) → review-
Comment on attachment 8458009 [details] [diff] [review]
part 1: browser changes v.4

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

This looks fine to me. Thanks Jim!
Attachment #8458009 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) from comment #76)
> Comment on attachment 8457470 [details] [diff] [review]
> part 3 - test updates v.1
> 
> Review of attachment 8457470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm - browser_pdfjs_views.js and browser_pdfjs_main.js are failing for me
> when I run it without --e10s - it's complaining about a window leak:
> 
> 0:09.98 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/extensions/pdfjs/test/
> browser_pdfjs_views.js | leaked until shutdown [nsGlobalWindow #17 outer 
> http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf]
>  0:09.98 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/extensions/pdfjs/test/
> browser_pdfjs_views.js | leaked until shutdown [nsGlobalWindow #19 inner
> http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf
> http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf]
> 
> So I think we're holding onto something that we shouldn't... an interesting
> clue is that browser_pdfjs_savedialog.js exits just fine without --e10s.
> 
> Otherwise, the code looks fine - though I have some small suggestions.

I get this too on local runs, however if I up the timeout for leak checking in mochitest harness, the problem goes away. Also, on try, these leaks don't show up. This leak checking timeout is pretty brittle imo.

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#516


> 
> ::: browser/extensions/pdfjs/test/browser_pdfjs_main.js
> @@ +6,4 @@
> >  const RELATIVE_DIR = "browser/extensions/pdfjs/test/";
> >  const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
> >  
> > +let gTab = null;
> 
> Instead of adding another global, how about we pass the tab to runTests
> instead? We lose a global that way, and you don't have to clean it out in
> the cleanup function.
> 
> ::: browser/extensions/pdfjs/test/browser_pdfjs_views.js
> @@ +6,4 @@
> >  const RELATIVE_DIR = "browser/extensions/pdfjs/test/";
> >  const TESTROOT = "http://example.com/browser/" + RELATIVE_DIR;
> >  
> > +let gTab = null;
> 
> Same suggestion applies to get rid of the global and pass the tab to
> runTests instead.
> 
> @@ +26,4 @@
> >    });
> >  
> > +  gTab = gBrowser.addTab(TESTROOT + "file_pdfjs_test.pdf");
> > +  var newTabBrowser = gBrowser.getBrowserForTab(gTab);
> 
> While you're changing the rest of the vars, might as well turn this into a
> let too.

thanks, will update.
Comment on attachment 8458009 [details] [diff] [review]
part 1: browser changes v.4

>+    let remote = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                        .getInterface(Ci.nsIWebNavigation)
>+                        .QueryInterface(Ci.nsILoadContext)
>+                        .useRemoteTabs;

aWindow.gMultiProcessBrowser works here, doesn't it?

> XPCOMUtils.defineLazyModuleGetter(this, "CustomizationTabPreloader",
>                                   "resource:///modules/CustomizationTabPreloader.jsm");
> 
>-XPCOMUtils.defineLazyModuleGetter(this, "PdfJs",
>-                                  "resource://pdf.js/PdfJs.jsm");

>+    let PdfJs = Cu.import("resource://pdf.js/PdfJs.jsm", {}).PdfJs;

Not quite sure why you replaced the lazy import with a local one. Your code is more self-contained now, but I'm not sure that's a useful optimization here. It surely is inconsistent with the other module imports in this file.
Attachment #8458009 - Flags: feedback-
Attached patch part 4 - test updates v.2 (obsolete) — Splinter Review
Updated per comments. I didn't do anything about the local leaks because I'm not convinced it's a real problem.
Attachment #8457470 - Attachment is obsolete: true
Attachment #8458043 - Flags: review?(mconley)
- updated per dao's nits.
Attachment #8458009 - Attachment is obsolete: true
Comment on attachment 8458044 [details] [diff] [review]
part 1: browser changes v.5 (r=mconley)

>+      // child only: similar to the call above for parent - inits settings,
>+      // register content handler, and init message manager child shim for
>+      // privileged api access. With older versions of the extension installed,
>+      // this load will fail passively.
>+      aWindow.messageManager.loadFrameScript("resource://pdf.js/pdfjschildbootstrap.js", true);

You also might want to document why this only needs to happen once and not for every window. To me at least this isn't quite clear.
Attachment #8458044 - Flags: review+
Comment on attachment 8458043 [details] [diff] [review]
part 4 - test updates v.2

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

Alright - if you're certain the leakcheck thing isn't a problem, then I'm good with this.
Attachment #8458043 - Flags: review?(mconley) → review+
try push with the latest set:
https://tbpl.mozilla.org/?tree=Try&rev=30769fc8bcf8
Keywords: sec-audit
Per yury's comment via email and I've confirmed locally, something cpow related that landed post my work here is causing tabs to crash when invoking the find bar. Digging into this.
Attached file stacks.txt (obsolete) —
(In reply to Jim Mathies [:jimm] from comment #87)
> Created attachment 8473033 [details]
> stacks.txt

<billm> jimm: I saw the stacks you posted in the pdf.js bug. are you hitting that event dispatching assertion?
<jimm> yeah. any ideas there? seems odd a cpow is in use for that event. 
<jimm> the event is a custom event the findbar fires
<jimm> http://hg.mozilla.org/mozilla-central/rev/2af93a18b6b1#l5.113
<jimm> not sure why that ends up in DispatchUrgentMessage, its just a simple dom event
<billm> jimm: is it possible that document is a CPOW?
<jimm> it's not, that's the find bar
<jimm> I dumped that initially, it was a xul document
<blassey> sorry for the calendar spam everyone
<blassey> I just accidently canceled the e10s standup
<billm> jimm: actually, could there be an event listener for that event that's then doing some CPOW stuff?
<jimm> pdf.js listens for it over in the content process, those events were added for pdf.js
<jimm> I haven't looks at the handler since it doesn't appear to get that far
<billm> but could an event handler be firing in the parent?
<billm> also, how is the event expected to get from the parent to the child?
<jimm> hmm, maybe pdf.js is getting a findbar cpow, and calling addeventlistener on it? 
<billm> oh, yeah, that's possible
<jimm> would that trigger the behavior we see in those stacks?
<jimm> I guess I should look at the event target in the child process, figure out where it's getting sent.
<billm> maybe. the weird thing is that the child is dispatching its own event and seems to be going through a script.
<billm> if you could get a JS stack trace in the child it might help
<jimm> billm: http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#958
<jimm> with my patch set applied, that findbar object is a cpow
<jimm> http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#759
<jimm> I'm betting it has something to do with those event listeners
<billm> jimm: ok, that makes sense. so we actually dispatch the event in the child, but then at line 796 it tries to dispatch another event, and that's when we assert
<billm> jimm: I guess in this case we always expect contentWindow to be pdf.js content?
<jimm> billm: yes, looks like it. 
<jimm> http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#942
<jimm> chromeWindow is a cpow
<jimm> here's domWindow - 
<jimm> http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#127
<jimm> they get it from the channel for some reason.
<billm> jimm: do you think it would be okay to run these events asynchronously?
<jimm> I think so
<jimm> :)
<billm> jimm: ok, great
<billm> it's unfortunate this is happening since there's nothing really wrong with this code
<billm> our asserts are just overly conservative
<jimm> billm: do you know of an ieasy way to 'executeSoon' something in chrome that isn't test related?
<jimm> maybe wrapping that contentWindow.dispatchEvent(forward) in something like that would solve this
<billm> jimm: yeah, it's one of those things you have to copy and paste unfortunately
<billm> jimm: Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);
<billm> I guess that's not so bad
<jimm> ok, will dig into this, maybe I can add a helper for this to BrowserUtils 
<billm> that would be cool. thanks.
<jimm> thanks for the help!
Attached patch part 2 - pdfjs changes v.2 (obsolete) — Splinter Review
Making this follow up event async fixes the crash, and doesn't break findbar functionality both for local and remote content.

Here's the change - 

-    contentWindow.dispatchEvent(forward);
+    // Due to restrictions with cpow use, we can't dispatch
+    // dom events with an urgent message on the stack. So bounce
+    // this off the main thread to make it async. 
+    Services.tm.mainThread.dispatch(function () {
+      contentWindow.dispatchEvent(forward);
+    }, Ci.nsIThread.DISPATCH_NORMAL);
Attachment #8457467 - Attachment is obsolete: true
Attachment #8457467 - Flags: review?(ydelendik)
Attachment #8473171 - Flags: review?(ydelendik)
Attached patch part 2 - pdfjs changes v.2 (obsolete) — Splinter Review
sorry, posted the interdiff. Here's the the complete part 1.
Attachment #8473171 - Attachment is obsolete: true
Attachment #8473171 - Flags: review?(ydelendik)
Attachment #8473206 - Flags: review?(ydelendik)
Attachment #8473206 - Flags: review?(ydelendik)
Attached patch part 2 - pdfjs changes v.3 (obsolete) — Splinter Review
Attachment #8473206 - Attachment is obsolete: true
Attachment #8473033 - Attachment is obsolete: true
Attached patch part 2 - pdfjs changes v.1 (obsolete) — Splinter Review
splitting up the patches for pdfjs and the shims.
Attachment #8479033 - Attachment is obsolete: true
Attached patch part 3 - shims v.1 (obsolete) — Splinter Review
Attachment #8458043 - Attachment description: part 3 - test updates v.2 → part 4 - test updates v.2
Attached patch part 3 - shims v.1 (obsolete) — Splinter Review
- findbar support working again
Attachment #8479038 - Attachment is obsolete: true
Comment on attachment 8479036 [details] [diff] [review]
part 2 - pdfjs changes v.1

Retested findbar and notifications support, everything is working again with these updates.
Attachment #8479036 - Flags: review?(ydelendik)
Attachment #8479251 - Flags: review?(ydelendik)
Attachment #8479036 - Flags: review?(ydelendik)
Attachment #8479251 - Flags: review?(ydelendik)
yury has merged the pdfjs parts of this into hit git repo, so we're going to bring this down via a pdf.js update, which will also need to include changes outside of browser/extensions/pdfjs.
Depends on: 1064496
Comment on attachment 8479251 [details] [diff] [review]
part 3 - shims v.1

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

::: browser/extensions/pdfjs/content/pdfjschildbootstrap.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

PDF.js has Apache v2 license assigned. Do you want to keep this file MPL2?
(In reply to Yury Delendik (:yury) from comment #98)
> Comment on attachment 8479251 [details] [diff] [review]
> part 3 - shims v.1
> 
> Review of attachment 8479251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/pdfjs/content/pdfjschildbootstrap.js
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> PDF.js has Apache v2 license assigned. Do you want to keep this file MPL2?

Using Apache v2 seems fine with me.
Attachment #8458043 - Attachment is obsolete: true
Attachment #8479036 - Attachment is obsolete: true
Attachment #8479251 - Attachment is obsolete: true
Depends on: 1071709
https://hg.mozilla.org/mozilla-central/rev/d3a7f765152f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Woo! Nice work, everyone.
Is it safe to remove "PDF.js does not work: bug 942707" from https://wiki.mozilla.org/Electrolysis#Known_Issues?
(In reply to Nicholas Nethercote [:njn] from comment #104)
> Is it safe to remove "PDF.js does not work: bug 942707" from
> https://wiki.mozilla.org/Electrolysis#Known_Issues?

Thanks for the reminder.. that list was way old, updated!
Depends on: 1072350
Target Milestone: --- → Firefox 35
It happens here.
Nightly x64 2014-10-29.
Open http://www.mhzshop.com/shop/index~sid~6ebf025a222e5db124cb78b3556d6a51~cl~details~anid~6e554368739b2eae1.31261255.htm
The 2 PDF links in the middle ("Voir le certificat") give:

Error: TypeError: document.getElementById(...) is null
Source File: chrome://web-developer/content/overlay/javascript/overlay.js
Line: 7333
Error: NS_ERROR_NOT_AVAILABLE: Async version must be used
Source File: resource://gre/components/nsHelperAppDlg.js
Line: 209
Flags: qe-verify+
PDF files correctly opened and no errors displayed in the console using latest Nightly, build ID: 20141223030207.

Verified on Mac OS X 10.9.5, Ubuntu 12.04 32-bit and Windows 7 64-bit.
Status: RESOLVED → VERIFIED
Depends on: 1160058
No longer depends on: 1160058
You need to log in before you can comment on or make changes to this bug.