908954 - Remove try…catch inside wrapWidgetEventHandler once bug 503244 is fixed

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Matthew N. [:MattN]]

(Quoting Dão Gottwald [:dao] from comment bug 865916 comment 88)
> It seems like bug 862627 will happen eventually, so the workaround may not
> be worth it. But if you want to keep it, you should add a comment pointing
> to a bug so we'll eventually get rid of the workaround.

https://hg.mozilla.org/projects/ux/rev/5a7f22213ce9#l3.145

Comment 1

[Manish Goregaokar [:manishearth]]

Those interested in takign this bug: Leave a comment here and ask me if you need any further help.

The relevant code is here[1].

 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#2176

Comment 2

[Abhirav Kariya]

Attachment: bug-908954

Please check!

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8479366 [details] [diff] [review]
bug-908954

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

In the future, ask me for feedback -- MattN should probably review this :)

::: browser/components/customizableui/CustomizableUI.jsm
@@ -2181,5 @@
>      aWidget[aEventName] = function(...aArgs) {
> -      // Wrap inside a try...catch to properly log errors, until bug 862627 is
> -      // fixed, which in turn might help bug 503244.
> -      try {
> -        // Don't copy the function to the normalized widget object, instead

These comments probably shouldn't be removed.

@@ -2189,1 @@
>          return aWidget.implementation[aEventName].apply(aWidget.implementation,

Nit: de-indent

Comment 4

[Abhirav Kariya]

Attachment: bug908954

Please check!!

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8479366 [details] [diff] [review]
bug-908954

In the future, mark old patches as obsolete, please :)

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8479396 [details] [diff] [review]
bug908954

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

::: browser/components/customizableui/CustomizableUI.jsm
@@ +2178,5 @@
>        aWidget[aEventName] = null;
>        return;
>      }
>      aWidget[aEventName] = function(...aArgs) {
> +      // Don't copy the function to the normalized widget object, instead             // keep it on the original object provided to the API so that        

nit: This line isn't indented properly, also there's some extra whitespace at the end

@@ +2182,5 @@
> +      // Don't copy the function to the normalized widget object, instead             // keep it on the original object provided to the API so that        
> +      // additional methods can be implemented and used by the event
> +      // handlers
> +    return aWidget.implementation[aEventName].apply(aWidget.implementation,
> +			    aArgs);

Nit: The `aArgs` should be aligned with `aWidget` as it was previously.

Comment 7

[Abhirav Kariya]

Attachment: bug.908954

please check! (ignore previous submission )

Comment 8

[Abhirav Kariya]

Attachment: bug.908954

sorry for taking so many attempts (new to all this)

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8479398 [details] [diff] [review]
bug.908954

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

Fine if you take many attempts, I took a lot on my first bugs too :)

::: browser/components/customizableui/CustomizableUI.jsm
@@ +2179,5 @@
>        return;
>      }
>      aWidget[aEventName] = function(...aArgs) {
> +      // Don't copy the function to the normalized widget object, instead
> +      // keep it on the original object provided to the API so that        

Still have that extra whitespace at the end of the line :)

Comment 10

[Abhirav Kariya]

Attachment: bug.908954

ty

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8479404 [details] [diff] [review]
bug.908954

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

This patch still has the whitespace, and also doesn't apply to the original file -- it only contains the indentation change (not the try-catch change)

Comment 12

[Abhirav Kariya]

Attachment: bug-908954.patch

Removed try..catch and respective comment, with proper indentation.

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8479424 [details] [diff] [review]
bug-908954.patch

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

Looks good to me, though the aArgs could be aligned with the aWidget (not too necessary)

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8479424 [details] [diff] [review]
bug-908954.patch

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

I'm no longer convinced it's a good idea to remove the try…catch since an exception in a widget's event handler shouldn't break the flow of callers in CustomizableUI.jsm. For example, if a widget implements onCreated and throws, we won't finish running buildWidget which means we won't call |aWidget.instances.set(aDocument, node);|. That doesn't seem like a good thing. Blair, what do you think? Maybe we should just remove the comment?

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8479424 [details] [diff] [review]
bug-908954.patch

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

Good catch, MattN. My comment about this at bug 865916 comment 82 is no longer accurate, as we've since removed the other try/catches for these JS-implemented events (cf, bug 981418) and we're just relying on wrapWidgetEventHandler() to make them safe.

So yea, we should just remove the comment - keeping the try/catch and reportError() call.

Comment 16

[Manish Goregaokar [:manishearth]]

Alright, thanks.

Abhirav, could you make a patch that removes the first comment (Wrap...)? Sorry about the confusion, after this we'll find you a bug with something more interesting.

Comment 17

[Abhirav Kariya]

ok .. sorry I was inactive since 2 days .. ill submit as soon as possible

Comment 18

[Abhirav Kariya]

Attachment: Done

please explain how to restore the firefox build ..

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8481769 [details] [diff] [review]
Done

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

This patch adds a try-catch block, instead of removing it. You probably aren't on a clean patch queue.

https://developer.mozilla.org/en/docs/Mercurial_FAQ

https://developer.mozilla.org/en/docs/Mercurial_Queues

Ping me in IRC if you want further help.

Comment 20

[Kapil Bakshi[geekspark]]

Attachment: patch.diff

I hope this solves it.

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8570246 [details] [diff] [review]
patch.diff

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

Looks good. Could you add a commit message too?

Instructions [here](https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F)

Comment 22

[Kapil Bakshi[geekspark]]

Attachment: patch_1.diff

Commit message has been added .

Comment 23

[Manish Goregaokar [:manishearth]]

I don't see any commit message.

Are you using Git?

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570246 [details] [diff] [review]
patch.diff

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

This patch doesn't remove the try/catch wrapping, it just removes the comment referencing it.

Comment 25

[Manish Goregaokar [:manishearth]]

@jaws: See comment 15

Comment 26

[Kapil Bakshi[geekspark]]

yes I am using git.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570246 [details] [diff] [review]
patch.diff

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

Ah, well then :)

Comment 28

[Kapil Bakshi[geekspark]]

Attachment: patch_1.diff

added the commit message .

Comment 29

[Carsten Book [:Tomcat]]

hi, this failed to apply:

unable to find 'components/customizableui/CustomizableUI.jsm' for patching
1 out of 1 hunks FAILED -- saving rejects to file components/customizableui/CustomizableUI.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh patch_1.diff

could you take a look, thanks!

Comment 30

[aarushivij]

Hello, Can I submit the patch for this ?
Reading the above comments, I have to remove the following comment https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#2857 ?
Thanks :)
Aarushi

Comment 31

[Matthew N. [:MattN]]

Yes, you can rebase the patch from comment 28.

Comment 32

[aarushivij]

Attachment: Bug 908954 - Remove try…catch comment inside wrapWidgetEventHandler r=MattN

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a427b90947bf
Remove try…catch comment inside wrapWidgetEventHandler once bug 503244 is fixed r=MattN

Comment 34

[Dorel Luca [:dluca]]

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